fix: walk contentSchema, await the error envelope, correct the README claim
Copilot review round 7. - Add `contentSchema` to the traversal. It holds the schema the decoded string content must satisfy, so a bare `true` or a remote `$ref` under it is a real finding all three clients were walking past. Also admit the three content keywords to `CONSTRAINING_KEYWORDS` on the same documented grounds as `format` — annotation-only by vocabulary, but plainly an attempt to constrain, so reporting such a schema as accepting anything would be noise. The comment now names that category rather than singling `format` out. - Make `handleError` async and await the envelope write. Round 6 made the `--strict` report await its write and stopped one line short: the envelope emitted for the very same throw still raced `process.exit()`, so the machine-readable line a CI caller parses could be lost on a pipe. The launcher's comment claiming the envelope was "well inside the pipe buffer" was wrong reasoning and is replaced — fitting in the buffer says nothing about whether the write was performed. Its call site now awaits; the bin's `.catch(handleError)` needed no change. - The CLI README called its sample block the "complete output" when the error envelope follows it on every non-zero exit. Now scoped to "the complete report", with the envelope named and linked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
488106f235
commit
31bdd532a6
6 changed files with 97 additions and 20 deletions
|
|
@ -164,8 +164,10 @@ on **stderr**, and exits `6` if any is error-severity:
|
||||||
mcp-inspector --cli <server> --method tools/list --strict
|
mcp-inspector --cli <server> --method tools/list --strict
|
||||||
```
|
```
|
||||||
|
|
||||||
Complete output against the `unportable-schemas-http.json` showcase — one
|
The complete **report** against the `unportable-schemas-http.json` showcase —
|
||||||
block per finding, then the summary:
|
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
|
```text
|
||||||
Error: tool "get_temp"
|
Error: tool "get_temp"
|
||||||
|
|
|
||||||
|
|
@ -19,15 +19,19 @@ describe("handleError", () => {
|
||||||
vi.restoreAllMocks();
|
vi.restoreAllMocks();
|
||||||
});
|
});
|
||||||
|
|
||||||
it("emits a JSON error envelope on stderr and exits with the classified code", () => {
|
it("emits a JSON error envelope on stderr and exits with the classified code", async () => {
|
||||||
const writeSpy = vi
|
const writeSpy = vi.spyOn(process.stderr, "write").mockImplementation(((
|
||||||
.spyOn(process.stderr, "write")
|
_chunk: unknown,
|
||||||
.mockImplementation((() => true) as never);
|
cb?: () => void,
|
||||||
|
) => {
|
||||||
|
cb?.();
|
||||||
|
return true;
|
||||||
|
}) as never);
|
||||||
const exitSpy = vi
|
const exitSpy = vi
|
||||||
.spyOn(process, "exit")
|
.spyOn(process, "exit")
|
||||||
.mockImplementation((() => undefined) as never);
|
.mockImplementation((() => undefined) as never);
|
||||||
|
|
||||||
handleError(new Error("boom"));
|
await handleError(new Error("boom"));
|
||||||
|
|
||||||
expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE);
|
expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.USAGE);
|
||||||
const written = writeSpy.mock.calls[0]![0] as string;
|
const written = writeSpy.mock.calls[0]![0] as string;
|
||||||
|
|
@ -38,18 +42,49 @@ describe("handleError", () => {
|
||||||
expect(parsed.error.message).toBe("boom");
|
expect(parsed.error.message).toBe("boom");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("uses a CliExitCodeError's exitCode and envelope code", () => {
|
it("uses a CliExitCodeError's exitCode and envelope code", async () => {
|
||||||
vi.spyOn(process.stderr, "write").mockImplementation((() => true) as never);
|
vi.spyOn(process.stderr, "write").mockImplementation(((
|
||||||
|
_chunk: unknown,
|
||||||
|
cb?: () => void,
|
||||||
|
) => {
|
||||||
|
cb?.();
|
||||||
|
return true;
|
||||||
|
}) as never);
|
||||||
const exitSpy = vi
|
const exitSpy = vi
|
||||||
.spyOn(process, "exit")
|
.spyOn(process, "exit")
|
||||||
.mockImplementation((() => undefined) as never);
|
.mockImplementation((() => undefined) as never);
|
||||||
|
|
||||||
handleError(
|
await handleError(
|
||||||
new CliExitCodeError(EXIT_CODES.NO_APP, "no app", { code: "no_app" }),
|
new CliExitCodeError(EXIT_CODES.NO_APP, "no app", { code: "no_app" }),
|
||||||
);
|
);
|
||||||
|
|
||||||
expect(exitSpy).toHaveBeenCalledWith(EXIT_CODES.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", () => {
|
describe("classifyError", () => {
|
||||||
|
|
|
||||||
|
|
@ -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
|
* Exit-code map. Non-zero codes let an automated caller (CI, an agent) branch
|
||||||
* on the failure class without regex-scraping stderr:
|
* 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<never> {
|
||||||
const { exitCode, stderr } = formatErrorOutput(error);
|
const { exitCode, stderr } = formatErrorOutput(error);
|
||||||
process.stderr.write(stderr);
|
await awaitableError(stderr);
|
||||||
process.exit(exitCode);
|
process.exit(exitCode);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -79,10 +79,12 @@ async function run(): Promise<void> {
|
||||||
// this `undefined`; fall through to the generic sink with the real message
|
// this `undefined`; fall through to the generic sink with the real message
|
||||||
// rather than throwing "handleError is not a function" over it.
|
// rather than throwing "handleError is not a function" over it.
|
||||||
if (typeof handleError !== "function") throw err;
|
if (typeof handleError !== "function") throw err;
|
||||||
// write-then-exit like the direct bin; the envelope is a few hundred bytes,
|
// Write-then-exit like the direct bin — and AWAIT it. The envelope being
|
||||||
// well inside the pipe buffer, so the truncation risk noted above (which is
|
// a few hundred bytes is not the safeguard it looks like: on a pipe the
|
||||||
// about a large stdout payload) doesn't apply to this stderr line.
|
// write is asynchronous, so `process.exit()` discards it whether or not
|
||||||
handleError(err); // writes the envelope + process.exit(code); never returns
|
// 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 {
|
} else {
|
||||||
const { runTui } = await import(clientEntry("tui"));
|
const { runTui } = await import(clientEntry("tui"));
|
||||||
|
|
|
||||||
|
|
@ -212,6 +212,20 @@ describe("lintToolSchemas — boolean-schema", () => {
|
||||||
{ type: "object", propertyNames: true, properties: {} },
|
{ type: "object", propertyNames: true, properties: {} },
|
||||||
"inputSchema.propertyNames",
|
"inputSchema.propertyNames",
|
||||||
],
|
],
|
||||||
|
[
|
||||||
|
"contentSchema",
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
a: {
|
||||||
|
type: "string",
|
||||||
|
contentMediaType: "application/json",
|
||||||
|
contentSchema: true,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
"inputSchema.properties.a.contentSchema",
|
||||||
|
],
|
||||||
[
|
[
|
||||||
"a dependentSchemas entry",
|
"a dependentSchemas entry",
|
||||||
{ type: "object", dependentSchemas: { a: true }, properties: {} },
|
{ type: "object", dependentSchemas: { a: true }, properties: {} },
|
||||||
|
|
@ -402,6 +416,9 @@ describe("lintToolSchemas — untyped-schema", () => {
|
||||||
["contains", { contains: { type: "string" } }],
|
["contains", { contains: { type: "string" } }],
|
||||||
["propertyNames", { propertyNames: { pattern: "^a" } }],
|
["propertyNames", { propertyNames: { pattern: "^a" } }],
|
||||||
["dependentRequired", { dependentRequired: { a: ["b"] } }],
|
["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) => {
|
])("does not flag a schema constrained only by %s", (_label, sub) => {
|
||||||
const findings = lintToolSchemas(
|
const findings = lintToolSchemas(
|
||||||
tool({ inputSchema: { type: "object", properties: { a: sub } } }),
|
tool({ inputSchema: { type: "object", properties: { a: sub } } }),
|
||||||
|
|
|
||||||
|
|
@ -92,6 +92,10 @@ const SUBSCHEMA_KEYWORDS = [
|
||||||
"unevaluatedProperties",
|
"unevaluatedProperties",
|
||||||
"additionalItems",
|
"additionalItems",
|
||||||
"unevaluatedItems",
|
"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;
|
] as const;
|
||||||
|
|
||||||
/** Keywords whose value is an array of subschemas. */
|
/** 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
|
* `$schema`, `$anchor`, `$vocabulary`: they hold or name subschemas for
|
||||||
* *other* schemas to reference, and constrain the current instance not at all.
|
* *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
|
* One category is admitted despite being annotation-only in 2020-12's default
|
||||||
* an annotation rather than an assertion. A schema declaring one is plainly an
|
* vocabulary: `format`, and the three content keywords (`contentMediaType`,
|
||||||
* attempt to constrain, and reporting it as accepting anything would be noise
|
* `contentEncoding`, `contentSchema`). A schema declaring any of them is
|
||||||
* rather than a finding an author can act on.
|
* 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
|
* Defining the rule this way, rather than as "has no `type`", is also
|
||||||
* deliberate: `{"properties": {…}}` with no `type` does constrain its input,
|
* deliberate: `{"properties": {…}}` with no `type` does constrain its input,
|
||||||
|
|
@ -160,6 +165,9 @@ const CONSTRAINING_KEYWORDS: ReadonlySet<string> = new Set([
|
||||||
"minLength",
|
"minLength",
|
||||||
"pattern",
|
"pattern",
|
||||||
"format",
|
"format",
|
||||||
|
"contentMediaType",
|
||||||
|
"contentEncoding",
|
||||||
|
"contentSchema",
|
||||||
"maxItems",
|
"maxItems",
|
||||||
"minItems",
|
"minItems",
|
||||||
"uniqueItems",
|
"uniqueItems",
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue