feat: flag unportable tool schemas in all three clients
Closes #1005 A tool schema can be perfectly legal JSON Schema and still be rejected by the client the server is meant to run against — Go's `jsonschema` emits a bare `true` for `interface{}`, which the SDK accepts and Claude Code refuses with an opaque "Invalid input". The Inspector passed it through silently. Adds `core/json/schemaLint.ts`, a portability lint over a tool's inputSchema/outputSchema, and surfaces one shared verdict everywhere: the CLI's `--strict` report (exit 6 on an error-severity finding), the TUI's tool list marker + detail pane, and the web Tools sidebar flag + detail section. Deliberately not a JSON Schema validator: a census of 617 public servers found zero that fail the SDK's own parse, so a conformance check would report nothing on real servers. Each rule is instead a construct that is legal JSON Schema and known to be refused or mishandled by a shipping client — a bare boolean in a schema position, an array-form `type`, a remote `$ref`, a schema with no validation keyword, a non-object root. `test-servers/configs/unportable-schemas-http.json` exercises every rule, via a new `rawToolSchemas` override — the Zod-built presets cannot emit any of these shapes, which is the same reason a real server only hits this when its schemas come from another generator. 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
2b129087a3
commit
2f9e0901c1
7 changed files with 1979 additions and 13 deletions
|
|
@ -0,0 +1,86 @@
|
|||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { expect, within } from "storybook/test";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { lintToolSchemas } from "@inspector/core/json/schemaLint.js";
|
||||
import { SchemaFindingsList } from "./SchemaFindingsList";
|
||||
|
||||
/** Lint a schema fixture the way the real panels do, so the stories show the
|
||||
* module's own wording rather than hand-written findings that could drift. */
|
||||
function findingsFor(tool: Partial<Tool>) {
|
||||
return lintToolSchemas({ name: "info", ...tool } as Tool);
|
||||
}
|
||||
|
||||
const meta: Meta<typeof SchemaFindingsList> = {
|
||||
title: "Elements/SchemaFindingsList",
|
||||
component: SchemaFindingsList,
|
||||
};
|
||||
|
||||
export default meta;
|
||||
type Story = StoryObj<typeof SchemaFindingsList>;
|
||||
|
||||
/** The reported case: Go's `jsonschema` emits `true` for an `interface{}`. */
|
||||
export const BareTrueProperty: Story = {
|
||||
args: {
|
||||
findings: findingsFor({
|
||||
outputSchema: {
|
||||
type: "object",
|
||||
properties: { data: true, topic: { type: "string" } },
|
||||
},
|
||||
} as Partial<Tool>),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await expect(canvas.getByText("Schema portability (1)")).toBeVisible();
|
||||
await expect(canvas.getByText("error")).toBeVisible();
|
||||
await expect(
|
||||
canvas.getByText("outputSchema.properties.data"),
|
||||
).toBeVisible();
|
||||
},
|
||||
};
|
||||
|
||||
/** Warning-only: an array-form `type`, legal but read unevenly. */
|
||||
export const TypeUnionWarning: Story = {
|
||||
args: {
|
||||
findings: findingsFor({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { show_ids: { type: ["null", "boolean"] } },
|
||||
},
|
||||
} as Partial<Tool>),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await expect(canvas.getByText("warning")).toBeVisible();
|
||||
await expect(canvas.queryByText("error")).toBeNull();
|
||||
},
|
||||
};
|
||||
|
||||
/** Both severities on one tool, which is what a real problem server looks like. */
|
||||
export const Mixed: Story = {
|
||||
args: {
|
||||
findings: findingsFor({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: {
|
||||
show_ids: { type: ["null", "boolean"] },
|
||||
extra: {},
|
||||
ref: { $ref: "https://example.com/s.json" },
|
||||
},
|
||||
},
|
||||
outputSchema: { type: "object", properties: { data: true } },
|
||||
} as Partial<Tool>),
|
||||
},
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await expect(canvas.getByText("Schema portability (4)")).toBeVisible();
|
||||
},
|
||||
};
|
||||
|
||||
/** The common case — nothing to say, so the section renders nothing at all. */
|
||||
export const Clean: Story = {
|
||||
args: { findings: [] },
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
await expect(canvas.queryByTestId("schema-findings")).toBeNull();
|
||||
},
|
||||
};
|
||||
|
|
@ -0,0 +1,61 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { screen } from "@testing-library/react";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { lintToolSchemas } from "@inspector/core/json/schemaLint.js";
|
||||
import { renderWithMantine } from "../../../test/renderWithMantine";
|
||||
import { SchemaFindingsList } from "./SchemaFindingsList";
|
||||
|
||||
function findingsFor(tool: Partial<Tool>) {
|
||||
return lintToolSchemas({ name: "info", ...tool } as Tool);
|
||||
}
|
||||
|
||||
describe("SchemaFindingsList", () => {
|
||||
it("renders nothing when there are no findings", () => {
|
||||
renderWithMantine(<SchemaFindingsList findings={[]} />);
|
||||
expect(screen.queryByTestId("schema-findings")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows the path, severity, issue and fix for an error finding", () => {
|
||||
renderWithMantine(
|
||||
<SchemaFindingsList
|
||||
findings={findingsFor({
|
||||
outputSchema: { type: "object", properties: { data: true } },
|
||||
} as Partial<Tool>)}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("Schema portability (1)")).toBeInTheDocument();
|
||||
expect(screen.getByText("error")).toBeInTheDocument();
|
||||
expect(
|
||||
screen.getByText("outputSchema.properties.data"),
|
||||
).toBeInTheDocument();
|
||||
expect(screen.getByText(/Bare `true`/)).toBeInTheDocument();
|
||||
expect(screen.getByText(/^Fix: /)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("labels a warning finding as such", () => {
|
||||
renderWithMantine(
|
||||
<SchemaFindingsList
|
||||
findings={findingsFor({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: ["null", "boolean"] } },
|
||||
},
|
||||
} as Partial<Tool>)}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("warning")).toBeInTheDocument();
|
||||
expect(screen.queryByText("error")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders one block per finding", () => {
|
||||
renderWithMantine(
|
||||
<SchemaFindingsList
|
||||
findings={findingsFor({
|
||||
inputSchema: { type: "object", properties: { a: true, b: true } },
|
||||
} as Partial<Tool>)}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("Schema portability (2)")).toBeInTheDocument();
|
||||
expect(screen.getAllByText("error")).toHaveLength(2);
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,103 @@
|
|||
import { Code, Group, Stack, Text } from "@mantine/core";
|
||||
import {
|
||||
describeSchemaPath,
|
||||
type SchemaFinding,
|
||||
} from "@inspector/core/json/schemaLint.js";
|
||||
|
||||
// Section wrapper: heading + one block per finding.
|
||||
const FindingsSection = Stack.withProps({
|
||||
gap: "xs",
|
||||
});
|
||||
|
||||
const FindingsTitle = Text.withProps({
|
||||
size: "sm",
|
||||
fw: 600,
|
||||
});
|
||||
|
||||
const FindingsNote = Text.withProps({
|
||||
size: "xs",
|
||||
c: "var(--inspector-text-secondary)",
|
||||
});
|
||||
|
||||
// One finding: severity badge + path on the first row, then issue and fix.
|
||||
const FindingBlock = Stack.withProps({
|
||||
gap: 2,
|
||||
});
|
||||
|
||||
const FindingHeadRow = Group.withProps({
|
||||
gap: "xs",
|
||||
wrap: "nowrap",
|
||||
align: "center",
|
||||
});
|
||||
|
||||
const FindingText = Text.withProps({
|
||||
size: "xs",
|
||||
c: "var(--inspector-text-secondary)",
|
||||
});
|
||||
|
||||
/**
|
||||
* Severity label.
|
||||
*
|
||||
* Deliberately coloured text rather than a filled `Badge`: a filled `yellow`
|
||||
* badge puts white on `yellow-7`, which at this size is 3.92:1 and fails the
|
||||
* story's a11y check, and neither `autoContrast` (yellow-7 sits below
|
||||
* Mantine's default luminance threshold, so it stays white) nor
|
||||
* `variant="light"` (3.34:1) fixes it. The `--inspector-*` severity tokens are
|
||||
* the pairings this app already uses against its own surfaces, in both colour
|
||||
* schemes.
|
||||
*/
|
||||
const SeverityLabel = Text.withProps({
|
||||
size: "xs",
|
||||
fw: 700,
|
||||
tt: "uppercase",
|
||||
});
|
||||
|
||||
/**
|
||||
* Severity → text-colour token. `error` is a construct a shipping MCP client
|
||||
* refuses outright; `warning` is one handled unevenly.
|
||||
*/
|
||||
function severityColor(severity: SchemaFinding["severity"]): string {
|
||||
return severity === "error"
|
||||
? "var(--inspector-danger-text)"
|
||||
: "var(--inspector-warning-text)";
|
||||
}
|
||||
|
||||
export interface SchemaFindingsListProps {
|
||||
/** Findings for one tool, in walk order. Renders nothing when empty. */
|
||||
findings: readonly SchemaFinding[];
|
||||
}
|
||||
|
||||
/**
|
||||
* Tool-schema portability findings for one tool (#1005).
|
||||
*
|
||||
* The same verdict the CLI's `--strict` report and the TUI's detail pane show
|
||||
* — all three read `core/json/schemaLint`, so they cannot disagree about
|
||||
* whether a schema is portable, only about how much room they have to say so.
|
||||
*/
|
||||
export function SchemaFindingsList({ findings }: SchemaFindingsListProps) {
|
||||
if (findings.length === 0) return null;
|
||||
|
||||
return (
|
||||
<FindingsSection data-testid="schema-findings">
|
||||
<FindingsTitle>Schema portability ({findings.length})</FindingsTitle>
|
||||
{findings.map((finding, index) => (
|
||||
<FindingBlock
|
||||
key={`${finding.schema}-${finding.path}-${finding.rule}-${index}`}
|
||||
>
|
||||
<FindingHeadRow>
|
||||
<SeverityLabel c={severityColor(finding.severity)}>
|
||||
{finding.severity}
|
||||
</SeverityLabel>
|
||||
<Code>{describeSchemaPath(finding.schema, finding.path)}</Code>
|
||||
</FindingHeadRow>
|
||||
<FindingText>{finding.issue}</FindingText>
|
||||
<FindingText>Fix: {finding.suggestion}</FindingText>
|
||||
</FindingBlock>
|
||||
))}
|
||||
<FindingsNote>
|
||||
These constructs are legal JSON Schema but are refused or mishandled by
|
||||
some MCP clients, so a tool can work here and fail there.
|
||||
</FindingsNote>
|
||||
</FindingsSection>
|
||||
);
|
||||
}
|
||||
|
|
@ -510,6 +510,30 @@ describe("ToolDetailPanel", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("schema portability findings (#1005)", () => {
|
||||
it("lists a finding with its path when a schema is unportable", () => {
|
||||
renderWithMantine(
|
||||
<ToolDetailPanel
|
||||
{...baseProps}
|
||||
tool={{
|
||||
name: "info",
|
||||
inputSchema: { type: "object", properties: {} },
|
||||
outputSchema: { type: "object", properties: { data: true } },
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("Schema portability (1)")).toBeInTheDocument();
|
||||
expect(
|
||||
screen.getByText("outputSchema.properties.data"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("omits the section for a portable tool", () => {
|
||||
renderWithMantine(<ToolDetailPanel {...baseProps} tool={simpleTool} />);
|
||||
expect(screen.queryByTestId("schema-findings")).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
// #2020: Execute used to be gated on `isExecuting` alone — this panel never
|
||||
// called `hasMissingRequiredFields`, unlike the App and elicitation panels —
|
||||
// so arguments that could not be sent were executed anyway and simply arrived
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ import {
|
|||
Switch,
|
||||
Text,
|
||||
} from "@mantine/core";
|
||||
import { useId, useState } from "react";
|
||||
import { useId, useMemo, useState } from "react";
|
||||
import { RiArrowDownSLine, RiArrowRightSLine } from "react-icons/ri";
|
||||
import type {
|
||||
ProgressNotification,
|
||||
|
|
@ -24,7 +24,9 @@ import {
|
|||
toFormSchema,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { getMirroredHeaderParams } from "@inspector/core/json/xMcpHeader.js";
|
||||
import { lintToolSchemas } from "@inspector/core/json/schemaLint.js";
|
||||
import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge";
|
||||
import { SchemaFindingsList } from "../../elements/SchemaFindingsList/SchemaFindingsList";
|
||||
import { ProgressDisplay } from "../../elements/ProgressDisplay/ProgressDisplay";
|
||||
import { SchemaForm } from "../SchemaForm/SchemaForm";
|
||||
|
||||
|
|
@ -219,6 +221,9 @@ export function ToolDetailPanel({
|
|||
// SEP-2243: args this tool declares as `x-mcp-header` — their values mirror
|
||||
// into `Mcp-Param-{Name}` headers on a `tools/call` (#1632).
|
||||
const mirroredParams = getMirroredHeaderParams(tool);
|
||||
// Memoized on the tool: this panel re-renders on every keystroke in the
|
||||
// argument form, and the walk depends on nothing that changes in between.
|
||||
const schemaFindings = useMemo(() => lintToolSchemas(tool), [tool]);
|
||||
|
||||
// Descriptions are shown by default (most are short); the chevron lets the
|
||||
// user hide a long one to keep the form and Execute footer in view. Reset to
|
||||
|
|
@ -338,6 +343,8 @@ export function ToolDetailPanel({
|
|||
</HeaderParamsSection>
|
||||
)}
|
||||
|
||||
<SchemaFindingsList findings={schemaFindings} />
|
||||
|
||||
<SchemaForm
|
||||
schema={formSchema}
|
||||
values={formValues}
|
||||
|
|
|
|||
|
|
@ -64,4 +64,48 @@ describe("ToolListItem", () => {
|
|||
);
|
||||
expect(screen.queryByRole("presentation")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows no schema flag for a clean tool", () => {
|
||||
renderWithMantine(
|
||||
<ToolListItem tool={tool} selected={false} onClick={() => {}} />,
|
||||
);
|
||||
expect(
|
||||
screen.queryByLabelText(/Schema portability/),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("flags a tool whose schema carries an error-severity finding", () => {
|
||||
renderWithMantine(
|
||||
<ToolListItem
|
||||
tool={{
|
||||
...tool,
|
||||
outputSchema: { type: "object", properties: { data: true } },
|
||||
}}
|
||||
selected={false}
|
||||
onClick={() => {}}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByLabelText("Schema portability errors"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("flags a warning-only tool distinctly", () => {
|
||||
renderWithMantine(
|
||||
<ToolListItem
|
||||
tool={{
|
||||
...tool,
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: ["null", "boolean"] } },
|
||||
},
|
||||
}}
|
||||
selected={false}
|
||||
onClick={() => {}}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.getByLabelText("Schema portability warnings"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,16 @@
|
|||
import { Group, Image, Stack, Text, UnstyledButton } from "@mantine/core";
|
||||
import {
|
||||
Group,
|
||||
Image,
|
||||
Stack,
|
||||
Text,
|
||||
ThemeIcon,
|
||||
Tooltip,
|
||||
UnstyledButton,
|
||||
} from "@mantine/core";
|
||||
import { useMemo } from "react";
|
||||
import { RiErrorWarningLine } from "react-icons/ri";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import { lintToolSchemas } from "@inspector/core/json/schemaLint.js";
|
||||
import { resolveDisplayLabel } from "../../../utils/toolUtils";
|
||||
|
||||
export interface ToolListItemProps {
|
||||
|
|
@ -43,9 +54,20 @@ const ListItemButton = UnstyledButton.withProps({
|
|||
variant: "listItem",
|
||||
});
|
||||
|
||||
// Schema-portability flag (#1005). Sits in the row rather than only in the
|
||||
// detail panel so a problem tool is visible without clicking each one — the
|
||||
// findings are rare enough that a marked row means something.
|
||||
const SchemaFlagIcon = ThemeIcon.withProps({
|
||||
size: "xs",
|
||||
variant: "transparent",
|
||||
role: "img",
|
||||
});
|
||||
|
||||
export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
|
||||
const { name, title, icons } = tool;
|
||||
const iconSrc = icons?.[0]?.src;
|
||||
const findings = useMemo(() => lintToolSchemas(tool), [tool]);
|
||||
const hasError = findings.some((f) => f.severity === "error");
|
||||
|
||||
return (
|
||||
<ListItemButton
|
||||
|
|
@ -58,6 +80,23 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
|
|||
<ItemLabel>{resolveDisplayLabel(name, title)}</ItemLabel>
|
||||
{title && <ItemSubLabel>{name}</ItemSubLabel>}
|
||||
</ItemBody>
|
||||
{findings.length > 0 && (
|
||||
<Tooltip
|
||||
label={`${findings.length} schema portability finding${findings.length === 1 ? "" : "s"} — select the tool for detail`}
|
||||
withArrow
|
||||
>
|
||||
<SchemaFlagIcon
|
||||
color={hasError ? "red" : "yellow"}
|
||||
aria-label={
|
||||
hasError
|
||||
? "Schema portability errors"
|
||||
: "Schema portability warnings"
|
||||
}
|
||||
>
|
||||
<RiErrorWarningLine />
|
||||
</SchemaFlagIcon>
|
||||
</Tooltip>
|
||||
)}
|
||||
</Row>
|
||||
</ListItemButton>
|
||||
);
|
||||
|
|
|
|||
Loading…
Reference in a new issue