fix: count severities correctly, gate remote-ref on $id, order --strict checks

Copilot review round 5.

- The sidebar tooltip applied `hasError` to the TOTAL, so a tool with one
  error and three warnings announced as "4 schema portability errors" —
  a defect the previous round introduced while adding that wording, and
  worse than the vague text it replaced. Both the tooltip and the
  accessible label now carry the breakdown, formatted by a new shared
  `summarizeToolFindings` so the fourth place to phrase a severity count
  cannot drift from the other three.

- Suppress `remote-ref` when the document declares an `$id` anywhere.
  `$id` establishes a base URI, so with a root `$id` of
  `https://example.com/root` the ref `https://example.com/root#/$defs/x`
  resolves to THIS document and needs no fetch — the finding and its
  "inline the referenced schema" remediation would both be wrong.
  Classifying correctly needs full RFC 3986 resolution against nested
  embedded resources, which this module does not do, so it declines
  rather than guessing. A missed finding is the acceptable direction.

- Run the `--strict` validations before every short-circuit return.
  `servers/list`, `servers/show`, `--list-stored-auth` and
  `--print-handoff` all return from `parseArgs` before any connect, so
  the checks further down let `--strict --method servers/list` succeed
  while silently ignoring a tools/list-only flag.

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>

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

clients/cli/src/cli.ts

@@ -829,6 +829,27 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     }
   }
 
+  // `--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 →
   // <MCP_STORAGE_DIR>/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<ParseResult> {
     );
   }
 
-  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).

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(
+      <ToolListItem
+        tool={{
+          ...tool,
+          inputSchema: {
+            type: "object",
+            properties: { a: { type: ["null", "boolean"] } },
+          },
+          outputSchema: { type: "object", properties: { data: true } },
+        }}
+        selected={false}
+        onClick={() => {}}
+      />,
+    );
+    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();
   });
 });

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 (
     <ListItemButton
@@ -87,7 +93,7 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
         </ItemBody>
         {findings.length > 0 && (
           <Tooltip
-            label={`${findings.length} schema portability ${hasError ? "error" : "warning"}${findings.length === 1 ? "" : "s"} — select the tool for detail`}
+            label={`Schema portability: ${summary} — select the tool for detail`}
             withArrow
           >
             <SchemaFlagIcon
@@ -96,11 +102,7 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
                   ? "var(--inspector-danger-text)"
                   : "var(--inspector-warning-text)"
               }
-              aria-label={
-                hasError
-                  ? "Schema portability errors"
-                  : "Schema portability warnings"
-              }
+              aria-label={`Schema portability: ${summary}`}
             >
               {/* Distinct SHAPES, not just distinct colours: an octagon for an
                   error and a triangle for a warning, so the severity survives

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");

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