fix: scan only subschema positions, await stderr, show the whole report

Copilot review round 6.

- Share one traversal between `walk` and `declaresAnyId`. The `$id` guard
  added last round descended through EVERY object value, including
  instance data — `examples: [{"$id": "x"}]`, a `default`, a `const` —
  where `$id` is a payload key declaring no embedded resource. Reading
  one there suppressed `remote-ref` for the whole document, turning last
  round's false-positive fix into a blanket false negative. Both passes
  now go through `forEachSubschema`, driven by the same keyword tables,
  so they cannot drift about what counts as a schema position.

- Await the stderr writes. `writeSchemaLintReport` used a bare
  `process.stderr.write`, but both CLI exit paths call `process.exit()`
  as soon as the work returns, which discards anything still buffered on
  a piped stderr — truncating or losing the multi-block `--strict`
  report on exactly the redirected-output runs a CI caller uses. Added
  `awaitableError` beside the existing `awaitableLog` and awaited it.
  The test's fake defers its callback so a synchronous one cannot make
  the assertion vacuous; two existing stderr stubs that never invoked
  their callbacks were fixed in the same pass.

- Show the CLI README's sample report in full. It printed one error
  block immediately followed by the summary, a shape the formatter
  cannot produce — every finding gets a block. Captured from a real run.

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:
cliffhall 2026-08-25 03:10:37 -04:00
commit 488106f235
6 changed files with 166 additions and 46 deletions

View file

@ -164,12 +164,30 @@ on **stderr**, and exits `6` if any is error-severity:
mcp-inspector --cli <server> --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/<name>`.
1 error, 3 warnings across 3 tools.
```

View file

@ -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;
});

View file

@ -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(

View file

@ -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<void> {
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`);
}

View file

@ -5,3 +5,21 @@ export function awaitableLog(logValue: string): Promise<void> {
});
});
}
/**
* 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<void> {
return new Promise<void>((resolve) => {
process.stderr.write(logValue, () => {
resolve();
});
});
}

View file

@ -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({