diff --git a/clients/cli/__tests__/schema-lint-report.test.ts b/clients/cli/__tests__/schema-lint-report.test.ts index 431375c8..f2c5575a 100644 --- a/clients/cli/__tests__/schema-lint-report.test.ts +++ b/clients/cli/__tests__/schema-lint-report.test.ts @@ -255,6 +255,20 @@ describe("--strict argument validation", () => { ).rejects.toThrow("--strict requires --method tools/list."); }); + it.each([ + ["servers/list", ["--method", "servers/list"]], + ["--list-stored-auth", ["--method", "servers/list", "--list-stored-auth"]], + ])( + "is rejected on the %s short-circuit path, which never reaches the lint", + async (_label, extra) => { + // These return from `parseArgs` before any connect, so a validation + // placed further down would let `--strict` be accepted and ignored. + await expect( + runCli(["node", "cli", "--cli", "--strict", ...extra]), + ).rejects.toThrow("--strict requires --method tools/list."); + }, + ); + it("is rejected alongside --app-info rather than silently ignored", async () => { // `tools/list --app-info` returns NDJSON straight from `runMethod` and // never reaches `emitResult`, so accepting the pair would hand a CI caller diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 57300275..c80cd1ab 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -829,6 +829,27 @@ async function parseArgs(argv?: string[]): Promise { } } + // `--strict` is checked HERE, ahead of every short-circuit return below + // (`--list-stored-auth`, `--print-handoff`, `servers/list`, `servers/show`), + // rather than beside the other method-shaped validations further down. Those + // returns never reach the lint, so a later check would let + // `--strict --method servers/list` succeed while silently ignoring a flag + // documented as tools/list-only — the same "accepted but inert" failure the + // `--app-info` pairing rejection exists to prevent. + if (options.strict) { + if (options.method !== "tools/list") { + throw new Error("--strict requires --method tools/list."); + } + // `tools/list --app-info` returns NDJSON straight from `runMethod` and + // never reaches `emitResult`, where the lint runs. Accepting the pair + // would hand a CI caller a gate that can never fail. + if (options.appInfo) { + throw new Error( + "--strict cannot be combined with --app-info; run tools/list twice, once for each.", + ); + } + } + // State-path precedence (getStateFilePath): MCP_INSPECTOR_OAUTH_STATE_PATH → // /oauth.json → ~/.mcp-inspector/storage/oauth.json — the // same file the web backend writes, so tokens are shared across surfaces. @@ -1009,20 +1030,9 @@ async function parseArgs(argv?: string[]): Promise { ); } - if (options.strict && options.method !== "tools/list") { - throw new Error("--strict requires --method tools/list."); - } - - // `tools/list --app-info` takes a different path out of `runMethod` — it - // returns NDJSON that the caller writes directly, never reaching - // `emitResult` where the lint runs. Rejecting the pair is the honest - // outcome: silently accepting it would give a CI caller a `--strict` gate - // that can never fail. - if (options.strict && options.appInfo) { - throw new Error( - "--strict cannot be combined with --app-info; run tools/list twice, once for each.", - ); - } + // NOTE: `--strict`'s validations are deliberately NOT here — they run before + // the short-circuit returns further up, so a `servers/*` invocation cannot + // accept the flag and ignore it. // --tool-args-json passes arguments verbatim with no key=value coercion (so // `"012"` stays a string and nested objects work without shell escaping). diff --git a/clients/web/src/components/groups/ToolListItem/ToolListItem.test.tsx b/clients/web/src/components/groups/ToolListItem/ToolListItem.test.tsx index 5244ef4a..853d8d7b 100644 --- a/clients/web/src/components/groups/ToolListItem/ToolListItem.test.tsx +++ b/clients/web/src/components/groups/ToolListItem/ToolListItem.test.tsx @@ -86,7 +86,28 @@ describe("ToolListItem", () => { />, ); expect( - screen.getByLabelText("Schema portability errors"), + screen.getByLabelText("Schema portability: 1 error"), + ).toBeInTheDocument(); + }); + + it("breaks a mixed tool down rather than labelling the total", () => { + // One error plus one warning must not announce as "2 errors". + renderWithMantine( + {}} + />, + ); + expect( + screen.getByLabelText("Schema portability: 1 error, 1 warning"), ).toBeInTheDocument(); }); @@ -105,7 +126,7 @@ describe("ToolListItem", () => { />, ); expect( - screen.getByLabelText("Schema portability warnings"), + screen.getByLabelText("Schema portability: 1 warning"), ).toBeInTheDocument(); }); }); diff --git a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx index 16d9b7ae..d1feaf2d 100644 --- a/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx +++ b/clients/web/src/components/groups/ToolListItem/ToolListItem.tsx @@ -10,7 +10,10 @@ import { import { useMemo } from "react"; import { RiAlertLine, RiErrorWarningLine } from "react-icons/ri"; import type { Tool } from "@modelcontextprotocol/client"; -import { lintToolSchemas } from "@inspector/core/json/schemaLint.js"; +import { + lintToolSchemas, + summarizeToolFindings, +} from "@inspector/core/json/schemaLint.js"; import { resolveDisplayLabel } from "../../../utils/toolUtils"; export interface ToolListItemProps { @@ -73,6 +76,9 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) { const iconSrc = icons?.[0]?.src; const findings = useMemo(() => lintToolSchemas(tool), [tool]); const hasError = findings.some((f) => f.severity === "error"); + // "1 error, 3 warnings" — the breakdown, not the total labelled with the + // worst severity, which would announce a mixed tool as "4 errors". + const summary = summarizeToolFindings(findings); return ( {findings.length > 0 && ( {/* Distinct SHAPES, not just distinct colours: an octagon for an error and a triangle for a warning, so the severity survives diff --git a/clients/web/src/test/core/schemaLint.test.ts b/clients/web/src/test/core/schemaLint.test.ts index 5a387fa2..b7866ddf 100644 --- a/clients/web/src/test/core/schemaLint.test.ts +++ b/clients/web/src/test/core/schemaLint.test.ts @@ -7,6 +7,7 @@ import { lintToolSchemas, lintTools, summarizeFindings, + summarizeToolFindings, type SchemaFinding, type SchemaLintRule, } from "@inspector/core/json/schemaLint.js"; @@ -453,6 +454,56 @@ describe("lintToolSchemas — remote-ref", () => { expect(findings[0]!.issue).toContain(ref); }); + it("stays quiet when the document declares an $id", () => { + // With a root `$id`, `https://example.com/root#/$defs/x` resolves to THIS + // document and needs no fetch — so the finding, and its "inline the + // referenced schema" remediation, would both be wrong. Classifying refs + // correctly here needs full base-URI resolution, which this module does + // not do, so it declines rather than guessing. + expect( + lintToolSchemas( + tool({ + inputSchema: { + $id: "https://example.com/root", + type: "object", + properties: { a: { $ref: "https://example.com/root#/$defs/x" } }, + $defs: { x: { type: "string" } }, + }, + }), + ), + ).toEqual([]); + }); + + it("finds an $id declared on a nested subschema too", () => { + // The base URI can be established by an embedded resource, not just the + // root, so the scan has to reach the whole document. + expect( + lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { + a: { $id: "https://example.com/embedded", type: "object" }, + b: { $ref: "https://example.com/embedded#/x" }, + }, + }, + }), + ), + ).toEqual([]); + }); + + it("still fires when no $id is declared anywhere", () => { + const findings = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { $ref: "https://example.com/root#/$defs/x" } }, + }, + }), + ); + expect(rules(findings)).toEqual(["remote-ref"]); + }); + it("ignores an empty $ref rather than calling it remote", () => { const findings = lintToolSchemas( tool({ @@ -606,6 +657,54 @@ describe("reporting helpers", () => { }); }); +describe("summarizeToolFindings", () => { + const mixed = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { + a: { type: ["null", "boolean"] }, + b: {}, + c: { $ref: "https://example.com/s.json" }, + }, + }, + outputSchema: { type: "object", properties: { data: true } }, + }), + ); + + it("breaks a mixed tool down instead of labelling the total", () => { + // The defect this replaced: one error plus three warnings announced as + // "4 schema portability errors". + expect(mixed).toHaveLength(4); + expect(summarizeToolFindings(mixed)).toBe("1 error, 3 warnings"); + }); + + it("omits a category with no findings", () => { + const warnOnly = lintToolSchemas( + tool({ + inputSchema: { + type: "object", + properties: { a: { type: ["null", "boolean"] } }, + }, + }), + ); + expect(summarizeToolFindings(warnOnly)).toBe("1 warning"); + }); + + it("reads as errors only when that is all there is", () => { + const errOnly = lintToolSchemas( + tool({ + inputSchema: { type: "object", properties: { a: true, b: true } }, + }), + ); + expect(summarizeToolFindings(errOnly)).toBe("2 errors"); + }); + + it("is empty for a clean tool", () => { + expect(summarizeToolFindings([])).toBe(""); + }); +}); + describe("describeSchemaPath", () => { it("names the schema alone at the root", () => { expect(describeSchemaPath("inputSchema", "")).toBe("inputSchema"); diff --git a/core/json/schemaLint.ts b/core/json/schemaLint.ts index 5b7147cb..4c38e0c0 100644 --- a/core/json/schemaLint.ts +++ b/core/json/schemaLint.ts @@ -283,6 +283,39 @@ export function describeSchemaPath(schema: SchemaKind, path: string): string { interface WalkContext { schema: SchemaKind; findings: SchemaFinding[]; + /** + * Whether this schema document declares an `$id` anywhere — see + * {@link declaresAnyId}. Suppresses the `remote-ref` rule. + */ + hasEmbeddedIds: boolean; +} + +/** + * Does the document declare an `$id` anywhere in it? + * + * `$id` establishes a base URI, which changes what "points outside this + * document" means: with a root `$id: "https://example.com/root"`, the `$ref` + * `"https://example.com/root#/$defs/x"` resolves to *this* document and needs + * no fetch at all. Classifying refs correctly in that case means full RFC 3986 + * base-URI resolution against possibly-nested embedded resources, which this + * module does not do — so when any `$id` is present it declines to classify + * rather than risk a wrong finding with wrong remediation on a valid schema. + * + * A missed finding is the acceptable direction here, the same trade the walk + * makes on a malformed `type` array. `$id` is vanishingly rare in a tool + * 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. + */ +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); + }); } function add( @@ -409,7 +442,12 @@ function lintNode( } const ref = node.$ref; - if (typeof ref === "string" && ref !== "" && !ref.startsWith("#")) { + if ( + typeof ref === "string" && + ref !== "" && + !ref.startsWith("#") && + !ctx.hasEmbeddedIds + ) { add( ctx, "remote-ref", @@ -459,7 +497,11 @@ function lintSchema( kind: SchemaKind, findings: SchemaFinding[], ): void { - walk(schema, "", undefined, 0, { schema: kind, findings }); + walk(schema, "", undefined, 0, { + schema: kind, + findings, + hasEmbeddedIds: declaresAnyId(schema), + }); } /** @@ -519,6 +561,26 @@ export function summarizeFindings( return `${plural(errors, "error")}, ${plural(warnings, "warning")} across ${plural(results.length, "tool")}`; } +/** + * Severity breakdown for ONE tool's findings — "1 error, 3 warnings". + * + * Deliberately not "N errors" keyed off the highest severity present: a tool + * with one error and three warnings is four findings, not four errors, and + * labelling the total with the worst severity misreports every mixed result. + * Categories that are empty are omitted, so a warning-only tool reads "3 + * warnings" rather than "0 errors, 3 warnings". + */ +export function summarizeToolFindings( + findings: readonly SchemaFinding[], +): string { + const errors = findings.filter((f) => f.severity === "error").length; + const warnings = findings.length - errors; + const parts: string[] = []; + if (errors > 0) parts.push(plural(errors, "error")); + if (warnings > 0) parts.push(plural(warnings, "warning")); + return parts.join(", "); +} + /** * Render a lint result as plain text — the CLI's `--strict` report, and the * body the TUI's detail pane prints. Shared so the two cannot describe the