Merge branch 'v2/main' into v2/fix/2117-persist-requested-scope
This commit is contained in:
Cliff Hall
GitHub
commit
6e88797416
9 changed files with 3230 additions and 27 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,69 @@ 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: 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();
|
||||
});
|
||||
|
||||
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: 1 warning"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,19 @@
|
|||
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 { RiAlertLine, RiErrorWarningLine } from "react-icons/ri";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
lintToolSchemas,
|
||||
summarizeToolFindings,
|
||||
} from "@inspector/core/json/schemaLint.js";
|
||||
import { resolveDisplayLabel } from "../../../utils/toolUtils";
|
||||
|
||||
export interface ToolListItemProps {
|
||||
|
|
@ -43,9 +57,28 @@ 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.
|
||||
//
|
||||
// Coloured from the same `--inspector-*` severity tokens the detail panel's
|
||||
// findings list uses, rather than from Mantine's `color="yellow"`: those are
|
||||
// the pairings this app already tunes against its own surfaces in both colour
|
||||
// schemes, and the default yellow is what failed the a11y check there.
|
||||
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");
|
||||
// "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
|
||||
|
|
@ -58,6 +91,27 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
|
|||
<ItemLabel>{resolveDisplayLabel(name, title)}</ItemLabel>
|
||||
{title && <ItemSubLabel>{name}</ItemSubLabel>}
|
||||
</ItemBody>
|
||||
{findings.length > 0 && (
|
||||
<Tooltip
|
||||
label={`Schema portability: ${summary} — select the tool for detail`}
|
||||
withArrow
|
||||
>
|
||||
<SchemaFlagIcon
|
||||
c={
|
||||
hasError
|
||||
? "var(--inspector-danger-text)"
|
||||
: "var(--inspector-warning-text)"
|
||||
}
|
||||
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
|
||||
for a colour-blind reader. The tooltip and the accessible
|
||||
label name it in words as well. */}
|
||||
{hasError ? <RiErrorWarningLine /> : <RiAlertLine />}
|
||||
</SchemaFlagIcon>
|
||||
</Tooltip>
|
||||
)}
|
||||
</Row>
|
||||
</ListItemButton>
|
||||
);
|
||||
|
|
|
|||
800
clients/web/src/test/core/schemaLint.test.ts
Normal file
800
clients/web/src/test/core/schemaLint.test.ts
Normal file
|
|
@ -0,0 +1,800 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import type { Tool } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
countFindings,
|
||||
describeSchemaPath,
|
||||
formatSchemaLintReport,
|
||||
lintToolSchemas,
|
||||
lintTools,
|
||||
summarizeFindings,
|
||||
summarizeToolFindings,
|
||||
type SchemaFinding,
|
||||
type SchemaLintRule,
|
||||
} from "@inspector/core/json/schemaLint.js";
|
||||
|
||||
/**
|
||||
* Minimal well-formed tool; each test overrides the schema under test.
|
||||
*
|
||||
* Overrides are `Record<string, unknown>` rather than `Partial<Tool>` on
|
||||
* purpose: the SDK types a tool schema as an *object* schema with a literal
|
||||
* `type: "object"`, and the whole point of this suite is to feed the lint the
|
||||
* malformed shapes a real server can send — an array-form `type`, a `properties`
|
||||
* that is not a map, a root that is a bare boolean. `Partial<Tool>` rejects
|
||||
* every one of them, so the fixture accepts the loose shape and narrows once,
|
||||
* here, instead of scattering a cast over each case.
|
||||
*/
|
||||
function tool(overrides: Record<string, unknown> = {}): Tool {
|
||||
return {
|
||||
name: "t",
|
||||
inputSchema: { type: "object", properties: {} },
|
||||
...overrides,
|
||||
} as Tool;
|
||||
}
|
||||
|
||||
/** The rules raised, in order — the shape most assertions here care about. */
|
||||
function rules(findings: readonly SchemaFinding[]): SchemaLintRule[] {
|
||||
return findings.map((f) => f.rule);
|
||||
}
|
||||
|
||||
function paths(findings: readonly SchemaFinding[]): string[] {
|
||||
return findings.map((f) => describeSchemaPath(f.schema, f.path));
|
||||
}
|
||||
|
||||
describe("lintToolSchemas — clean schemas", () => {
|
||||
it.each([
|
||||
["an empty object schema", { type: "object", properties: {} }],
|
||||
[
|
||||
"a typed property",
|
||||
{ type: "object", properties: { a: { type: "string" } } },
|
||||
],
|
||||
[
|
||||
"an enum property with no type",
|
||||
{ type: "object", properties: { a: { enum: ["x", "y"] } } },
|
||||
],
|
||||
[
|
||||
"a const property with no type",
|
||||
{ type: "object", properties: { a: { const: 3 } } },
|
||||
],
|
||||
[
|
||||
"a local $ref",
|
||||
{
|
||||
type: "object",
|
||||
properties: { a: { $ref: "#/$defs/x" } },
|
||||
$defs: { x: { type: "string" } },
|
||||
},
|
||||
],
|
||||
[
|
||||
"additionalProperties: true",
|
||||
{ type: "object", properties: {}, additionalProperties: true },
|
||||
],
|
||||
[
|
||||
"additionalProperties: false",
|
||||
{ type: "object", properties: {}, additionalProperties: false },
|
||||
],
|
||||
[
|
||||
"unevaluatedProperties / additionalItems / unevaluatedItems booleans",
|
||||
{
|
||||
type: "object",
|
||||
unevaluatedProperties: false,
|
||||
additionalItems: true,
|
||||
unevaluatedItems: false,
|
||||
},
|
||||
],
|
||||
[
|
||||
"a shape-implying schema with no type",
|
||||
{
|
||||
type: "object",
|
||||
properties: { a: { properties: { b: { type: "string" } } } },
|
||||
},
|
||||
],
|
||||
[
|
||||
"an anyOf branch set",
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
a: { anyOf: [{ type: "string" }, { type: "null" }] },
|
||||
},
|
||||
},
|
||||
],
|
||||
[
|
||||
"a typed array with typed items",
|
||||
{
|
||||
type: "object",
|
||||
properties: { a: { type: "array", items: { type: "number" } } },
|
||||
},
|
||||
],
|
||||
])("reports nothing for %s", (_label, inputSchema) => {
|
||||
expect(lintToolSchemas(tool({ inputSchema }))).toEqual([]);
|
||||
});
|
||||
|
||||
it("reports nothing when a tool declares neither schema", () => {
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({ inputSchema: undefined, outputSchema: undefined }),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — boolean-schema", () => {
|
||||
it("flags the Go interface{} case from the issue", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
outputSchema: {
|
||||
type: "object",
|
||||
properties: { data: true, topic: { type: "string" } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["boolean-schema"]);
|
||||
expect(paths(findings)).toEqual(["outputSchema.properties.data"]);
|
||||
expect(findings[0]!.severity).toBe("error");
|
||||
expect(findings[0]!.suggestion).toContain("additionalProperties");
|
||||
});
|
||||
|
||||
it("suggests `{}` for a bare true under not, where a typed schema would widen", () => {
|
||||
// `{"not": true}` rejects every value. Swapping in `{"type": "object", …}`
|
||||
// leaves the parent rejecting only objects, so every non-object would then
|
||||
// pass — the enclosing contract is widened, not narrowed. `{}` is the
|
||||
// exact replacement, and is exempt from `untyped-schema` in that position.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: { not: true } } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["boolean-schema"]);
|
||||
expect(findings[0]!.suggestion).toContain("exact replacement");
|
||||
expect(findings[0]!.suggestion).not.toContain('"additionalProperties"');
|
||||
});
|
||||
|
||||
it("never claims a bare-true replacement narrows the contract", () => {
|
||||
// It narrows in an ordinary property position and does not under `not` or
|
||||
// `if`, so the wording must not assert an effect it cannot guarantee.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: true, b: { not: true }, c: { if: true } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
// Four: a bare `true` in each of the three positions, plus `c` itself
|
||||
// being unconstrained — a lone `if` asserts nothing without a `then`.
|
||||
expect(rules(findings)).toEqual([
|
||||
"boolean-schema",
|
||||
"boolean-schema",
|
||||
"untyped-schema",
|
||||
"boolean-schema",
|
||||
]);
|
||||
for (const finding of findings) {
|
||||
expect(finding.suggestion).not.toMatch(/narrow/i);
|
||||
}
|
||||
});
|
||||
|
||||
it("suggests the always-false object form for a bare false", () => {
|
||||
// `properties: {a: false}` forbids the property. Deleting the entry would
|
||||
// *permit* it with any value under the default `additionalProperties`, so
|
||||
// the suggestion has to preserve the constraint rather than drop it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { data: false } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["boolean-schema"]);
|
||||
expect(findings[0]!.suggestion).toContain('{"not": {}}');
|
||||
expect(findings[0]!.suggestion).toContain("different contract");
|
||||
expect(findings[0]!.suggestion).not.toMatch(/^Remove the property/);
|
||||
});
|
||||
|
||||
it('does not flag the `{"not": {}}` form it recommends', () => {
|
||||
// The suggestion above would be self-defeating if this module reported its
|
||||
// own recommended replacement.
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { data: { not: {} } } },
|
||||
}),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("still flags an unconstrained schema elsewhere under a not branch", () => {
|
||||
// The exemption is scoped to the `not` position itself, not inherited by
|
||||
// everything beneath it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { not: { properties: { b: {} } } } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
expect(paths(findings)).toEqual([
|
||||
"inputSchema.properties.a.not.properties.b",
|
||||
]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
[
|
||||
"items",
|
||||
{ type: "object", properties: { a: { items: true } } },
|
||||
"inputSchema.properties.a.items",
|
||||
],
|
||||
[
|
||||
"an anyOf branch",
|
||||
{ type: "object", properties: { a: { anyOf: [true] } } },
|
||||
"inputSchema.properties.a.anyOf[0]",
|
||||
],
|
||||
[
|
||||
"a prefixItems entry",
|
||||
{ type: "object", properties: { a: { prefixItems: [true] } } },
|
||||
"inputSchema.properties.a.prefixItems[0]",
|
||||
],
|
||||
[
|
||||
"a $defs entry",
|
||||
{ type: "object", $defs: { x: true }, properties: {} },
|
||||
"inputSchema.$defs.x",
|
||||
],
|
||||
[
|
||||
"a patternProperties entry",
|
||||
{ type: "object", patternProperties: { "^a": true }, properties: {} },
|
||||
'inputSchema.patternProperties["^a"]',
|
||||
],
|
||||
[
|
||||
"a tuple-form items entry",
|
||||
{ type: "object", properties: { a: { items: [true] } } },
|
||||
"inputSchema.properties.a.items[0]",
|
||||
],
|
||||
[
|
||||
"propertyNames",
|
||||
{ type: "object", propertyNames: true, properties: {} },
|
||||
"inputSchema.propertyNames",
|
||||
],
|
||||
[
|
||||
"contentSchema",
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
a: {
|
||||
type: "string",
|
||||
contentMediaType: "application/json",
|
||||
contentSchema: true,
|
||||
},
|
||||
},
|
||||
},
|
||||
"inputSchema.properties.a.contentSchema",
|
||||
],
|
||||
[
|
||||
"a dependentSchemas entry",
|
||||
{ type: "object", dependentSchemas: { a: true }, properties: {} },
|
||||
"inputSchema.dependentSchemas.a",
|
||||
],
|
||||
[
|
||||
"a draft-07 schema-valued dependencies entry",
|
||||
{ type: "object", dependencies: { a: true }, properties: {} },
|
||||
"inputSchema.dependencies.a",
|
||||
],
|
||||
["the schema root", true, "inputSchema"],
|
||||
])("flags a bare boolean under %s", (_label, inputSchema, path) => {
|
||||
const findings = lintToolSchemas(tool({ inputSchema }));
|
||||
expect(rules(findings)).toEqual(["boolean-schema"]);
|
||||
expect(paths(findings)).toEqual([path]);
|
||||
});
|
||||
|
||||
it("quotes a non-identifier property name in the path", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { "odd key": true } },
|
||||
}),
|
||||
);
|
||||
expect(paths(findings)).toEqual(['inputSchema.properties["odd key"]']);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — type-union", () => {
|
||||
it("flags the nullable array form and suggests anyOf branches", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { show_ids: { type: ["null", "boolean"] } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["type-union"]);
|
||||
expect(findings[0]!.severity).toBe("warning");
|
||||
expect(findings[0]!.suggestion).toContain(
|
||||
'{"anyOf": [{"type": "null"}, {"type": "boolean"}]}',
|
||||
);
|
||||
});
|
||||
|
||||
it("never suggests un-requiring the property as the equivalent fix", () => {
|
||||
// Absent and `null` are different contracts, so "drop it from `required`"
|
||||
// is not a portable spelling of the same schema — it is a change to it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: ["null", "string"] } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(findings[0]!.suggestion).toContain("absent is not the same as");
|
||||
expect(findings[0]!.suggestion).not.toMatch(/leave the property out/);
|
||||
});
|
||||
|
||||
it("keeps every member in the suggestion when none is null", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: ["string", "number"] } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(findings[0]!.suggestion).toContain(
|
||||
'{"anyOf": [{"type": "string"}, {"type": "number"}]}',
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["an empty array, which matches nothing", []],
|
||||
["a non-string member", ["string", 3]],
|
||||
["only non-string members", [3]],
|
||||
["an unrecognized type name", ["string", "bananas"]],
|
||||
["a duplicated member", ["string", "string"]],
|
||||
])("stays quiet on a malformed type array — %s", (_label, type) => {
|
||||
// The rule's message asserts the construct is *legal* JSON Schema, and the
|
||||
// `anyOf` it suggests would be invalid for these. Malformed schemas are
|
||||
// the SDK parser's business, the same way a non-schema node is.
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({ inputSchema: { type: "object", properties: { a: { type } } } }),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["integer", ["integer", "null"]],
|
||||
["array", ["array", "null"]],
|
||||
["object", ["object", "string"]],
|
||||
])("still fires for a well-formed union containing %s", (_label, type) => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({ inputSchema: { type: "object", properties: { a: { type } } } }),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["type-union"]);
|
||||
});
|
||||
|
||||
it("raises only the union rule for an array-typed root", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({ inputSchema: { type: ["object", "null"] } }),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["type-union"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — untyped-schema", () => {
|
||||
it.each([
|
||||
["an empty schema", {}],
|
||||
["an annotation-only schema", { description: "anything at all" }],
|
||||
[
|
||||
"a title/default-only schema",
|
||||
{ title: "T", default: 1, examples: [1], deprecated: false },
|
||||
],
|
||||
])("flags %s", (_label, sub) => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: sub } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
expect(paths(findings)).toEqual(["inputSchema.properties.a"]);
|
||||
});
|
||||
|
||||
it("flags a schema whose only keyword is unrecognized", () => {
|
||||
// JSON Schema ignores keywords it does not know, so `{"vendorHint": true}`
|
||||
// accepts every value. A denylist of known annotations would let it pass
|
||||
// simply because the keyword is unfamiliar — hence the allowlist.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { vendorHint: true, "x-internal": 1 } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
});
|
||||
|
||||
it("flags a definition-only schema", () => {
|
||||
// `$defs` holds subschemas for *other* schemas to reference; it constrains
|
||||
// the current instance not at all.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { $defs: { x: { type: "string" } } } },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
expect(paths(findings)).toEqual(["inputSchema.properties.a"]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["a lone if", { if: { const: 1 } }],
|
||||
["a lone then", { then: { type: "string" } }],
|
||||
["a lone else", { else: { type: "string" } }],
|
||||
])("flags %s, which asserts nothing on its own", (_label, sub) => {
|
||||
// `if` has no assertion effect without `then`/`else`, and either of those
|
||||
// is ignored without an `if` — so these accept every value, and counting
|
||||
// the keyword's mere presence would let them pass as constrained.
|
||||
const findings = lintToolSchemas(
|
||||
tool({ inputSchema: { type: "object", properties: { a: sub } } }),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["if + then", { if: { const: 1 }, then: { type: "string" } }],
|
||||
["if + else", { if: { const: 1 }, else: { type: "string" } }],
|
||||
])("does not flag %s, which is a real conditional", (_label, sub) => {
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({ inputSchema: { type: "object", properties: { a: sub } } }),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["pattern", { pattern: "^a" }],
|
||||
["format", { format: "email" }],
|
||||
["required", { required: ["a"] }],
|
||||
["minimum", { minimum: 0 }],
|
||||
["contains", { contains: { type: "string" } }],
|
||||
["propertyNames", { propertyNames: { pattern: "^a" } }],
|
||||
["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) => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({ inputSchema: { type: "object", properties: { a: sub } } }),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
|
||||
it("flags an entirely empty root schema", () => {
|
||||
const findings = lintToolSchemas(tool({ inputSchema: {} }));
|
||||
expect(rules(findings)).toEqual(["untyped-schema"]);
|
||||
expect(paths(findings)).toEqual(["inputSchema"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — no root-type rule", () => {
|
||||
// There is deliberately no "inputSchema must be an object" rule: the SDK
|
||||
// types `inputSchema` with `type: literal("object")`, so such a tool fails
|
||||
// `ListToolsResultSchema` and `salvageListItems` drops it before any client
|
||||
// sees it. `raw-tool-schemas.test.ts` pins that live behaviour; these cases
|
||||
// pin that the module stays quiet rather than reporting a check nothing can
|
||||
// reach.
|
||||
it.each([
|
||||
["a non-object inputSchema root", { inputSchema: { type: "array" } }],
|
||||
["a non-object outputSchema root", { outputSchema: { type: "string" } }],
|
||||
[
|
||||
"a nested non-object schema",
|
||||
{
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: "array", items: { type: "string" } } },
|
||||
},
|
||||
},
|
||||
],
|
||||
])("reports nothing for %s", (_label, overrides) => {
|
||||
expect(lintToolSchemas(tool(overrides))).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — remote-ref", () => {
|
||||
it.each([
|
||||
["an https ref", "https://example.com/schema.json"],
|
||||
["a sibling-file ref", "common.json#/$defs/x"],
|
||||
])("flags %s", (_label, ref) => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: { $ref: ref } } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["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.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({
|
||||
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({
|
||||
inputSchema: { type: "object", properties: { a: { $ref: "" } } },
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintToolSchemas — walk robustness", () => {
|
||||
it("skips a value that is not a schema at all", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: "nonsense" } },
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
|
||||
it("skips the draft-07 property-name-array form of dependencies", () => {
|
||||
// `dependencies: {a: ["b"]}` is a required-property list, not a schema
|
||||
// position, so the walk must not descend into it.
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { a: { type: "string" }, b: { type: "string" } },
|
||||
dependencies: { a: ["b"] },
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
|
||||
it("skips keyword values of the wrong shape", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: "not-a-map",
|
||||
anyOf: "not-an-array",
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
|
||||
it("stops at the depth cap instead of recursing forever", () => {
|
||||
// 200 nested `items`, deeper than MAX_DEPTH (64), each level clean; the
|
||||
// bare `true` at the bottom sits past the cap and is therefore not
|
||||
// reported. The assertion is that the walk terminates and stays quiet.
|
||||
let nested: unknown = true;
|
||||
for (let i = 0; i < 200; i++) nested = { type: "array", items: nested };
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: nested } },
|
||||
}),
|
||||
);
|
||||
expect(findings).toEqual([]);
|
||||
});
|
||||
|
||||
it("walks both schemas, inputSchema first", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: true } },
|
||||
outputSchema: { type: "object", properties: { b: true } },
|
||||
}),
|
||||
);
|
||||
expect(paths(findings)).toEqual([
|
||||
"inputSchema.properties.a",
|
||||
"outputSchema.properties.b",
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lintTools", () => {
|
||||
it("drops clean tools and keeps the rest in list order", () => {
|
||||
const results = lintTools([
|
||||
tool({ name: "clean" }),
|
||||
tool({
|
||||
name: "dirty",
|
||||
inputSchema: { type: "object", properties: { a: true } },
|
||||
}),
|
||||
tool({ name: "also-clean" }),
|
||||
]);
|
||||
expect(results.map((r) => r.toolName)).toEqual(["dirty"]);
|
||||
expect(results[0]!.findings).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("returns an empty list when everything is clean", () => {
|
||||
expect(lintTools([tool(), tool({ name: "b" })])).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("reporting helpers", () => {
|
||||
const results = lintTools([
|
||||
tool({
|
||||
name: "info",
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { show_ids: { type: ["null", "boolean"] } },
|
||||
},
|
||||
outputSchema: { type: "object", properties: { data: true } },
|
||||
}),
|
||||
]);
|
||||
|
||||
it("counts by severity", () => {
|
||||
expect(countFindings(results)).toEqual({ errors: 1, warnings: 1 });
|
||||
});
|
||||
|
||||
it("counts nothing for an empty result", () => {
|
||||
expect(countFindings([])).toEqual({ errors: 0, warnings: 0 });
|
||||
});
|
||||
|
||||
it("summarizes with singular nouns at one", () => {
|
||||
expect(summarizeFindings(results)).toBe("1 error, 1 warning across 1 tool");
|
||||
});
|
||||
|
||||
it("pluralizes above one", () => {
|
||||
const two = lintTools([
|
||||
tool({
|
||||
name: "a",
|
||||
inputSchema: { type: "object", properties: { x: true, y: true } },
|
||||
}),
|
||||
tool({
|
||||
name: "b",
|
||||
inputSchema: { type: "object", properties: { x: true } },
|
||||
}),
|
||||
]);
|
||||
expect(summarizeFindings(two)).toBe("3 errors, 0 warnings across 2 tools");
|
||||
});
|
||||
|
||||
it("renders a report with a path, issue and suggestion per finding", () => {
|
||||
const report = formatSchemaLintReport(results);
|
||||
expect(report).toContain('Error: tool "info"');
|
||||
expect(report).toContain('Warning: tool "info"');
|
||||
expect(report).toContain("Path: outputSchema.properties.data");
|
||||
expect(report).toContain("Path: inputSchema.properties.show_ids");
|
||||
expect(report).toContain("Issue: ");
|
||||
expect(report).toContain("Suggestion: ");
|
||||
expect(report.trimEnd().endsWith("1 error, 1 warning across 1 tool.")).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it("renders a zero summary for an empty result", () => {
|
||||
expect(formatSchemaLintReport([])).toBe(
|
||||
"0 errors, 0 warnings across 0 tools.",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
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");
|
||||
});
|
||||
|
||||
it("joins a nested path onto the schema name", () => {
|
||||
expect(describeSchemaPath("outputSchema", "properties.a")).toBe(
|
||||
"outputSchema.properties.a",
|
||||
);
|
||||
});
|
||||
});
|
||||
329
clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts
Normal file
329
clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts
Normal file
|
|
@ -0,0 +1,329 @@
|
|||
import { describe, it, expect, afterEach } from "vitest";
|
||||
import path from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
|
||||
import { countFindings, lintTools } from "@inspector/core/json/schemaLint.js";
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
createTestServerInfo,
|
||||
createEchoTool,
|
||||
createGetWeatherTool,
|
||||
createGetTempTool,
|
||||
loadConfig,
|
||||
resolveConfig,
|
||||
} from "@modelcontextprotocol/inspector-test-server";
|
||||
import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server";
|
||||
|
||||
/**
|
||||
* Live coverage of `ServerConfig.rawToolSchemas` (#1005) — the only way this
|
||||
* repo can advertise a tool schema containing a bare `true`, an array-form
|
||||
* `type`, or a remote `$ref`, since every preset builds its schema from Zod
|
||||
* and Zod cannot emit any of them.
|
||||
*
|
||||
* The lint itself is unit-tested against hand-built `Tool` objects, which
|
||||
* bypasses this path entirely: handler installation, config resolution, and
|
||||
* the interaction with pagination and duplication could all regress while
|
||||
* those stayed green. This file drives the real server over a real transport
|
||||
* and asserts on what actually reaches the wire.
|
||||
*/
|
||||
describe("rawToolSchemas override (#1005)", () => {
|
||||
let client: InspectorClient | null = null;
|
||||
let server: TestServerHttp | null = null;
|
||||
|
||||
afterEach(async () => {
|
||||
if (client) {
|
||||
try {
|
||||
await client.disconnect();
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
client = null;
|
||||
}
|
||||
if (server) {
|
||||
try {
|
||||
await server.stop();
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
server = null;
|
||||
}
|
||||
});
|
||||
|
||||
async function start(config: Partial<ServerConfig>): Promise<TestServerHttp> {
|
||||
const started = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo("raw-tool-schemas-test", "1.0.0"),
|
||||
tools: [createEchoTool(), createGetWeatherTool()],
|
||||
...config,
|
||||
});
|
||||
await started.start();
|
||||
server = started;
|
||||
return started;
|
||||
}
|
||||
|
||||
async function connect(url: string): Promise<InspectorClient> {
|
||||
const connected = new InspectorClient(
|
||||
{ type: "streamable-http", url },
|
||||
{ environment: { transport: createTransportNode } },
|
||||
);
|
||||
await connected.connect();
|
||||
client = connected;
|
||||
return connected;
|
||||
}
|
||||
|
||||
it("advertises the raw document, and the lint finds it end to end", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: {
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: {
|
||||
message: { type: "string" },
|
||||
show_ids: { type: ["null", "boolean"] },
|
||||
},
|
||||
},
|
||||
outputSchema: {
|
||||
type: "object",
|
||||
properties: { data: true },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const echo = tools.find((t) => t.name === "echo");
|
||||
|
||||
// The bare `true` survives the wire — a Zod-built schema could not
|
||||
// produce it, so its presence here proves the override is what is served.
|
||||
expect(echo?.outputSchema).toEqual({
|
||||
type: "object",
|
||||
properties: { data: true },
|
||||
});
|
||||
expect(
|
||||
(echo?.inputSchema as { properties?: Record<string, unknown> }).properties
|
||||
?.show_ids,
|
||||
).toEqual({ type: ["null", "boolean"] });
|
||||
|
||||
const results = lintTools(tools);
|
||||
expect(results.map((r) => r.toolName)).toEqual(["echo"]);
|
||||
expect(results[0]?.findings.map((f) => f.rule).sort()).toEqual([
|
||||
"boolean-schema",
|
||||
"type-union",
|
||||
]);
|
||||
});
|
||||
|
||||
it("drops a tool whose inputSchema root is not an object, before any client sees it", async () => {
|
||||
// This is why `schemaLint` has no "inputSchema must be an object" rule.
|
||||
// The SDK types `inputSchema` with `type: literal("object")`, so such a
|
||||
// tool fails `ListToolsResultSchema` and `salvageListItems` removes it —
|
||||
// no client can pass it to the lint, and a rule for it could never fire.
|
||||
// If this ever starts returning the tool, that rule becomes worth adding
|
||||
// back and this test is the signal.
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: { inputSchema: { type: "array", items: { type: "string" } } },
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
expect(tools.map((t) => t.name)).toEqual(["get_weather"]);
|
||||
});
|
||||
|
||||
it("leaves an unnamed tool's real schema alone", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: { echo: { outputSchema: { type: "object" } } },
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const weather = tools.find((t) => t.name === "get_weather");
|
||||
// get_weather keeps the Zod-derived schema, which is portable.
|
||||
expect(weather?.inputSchema).toMatchObject({ type: "object" });
|
||||
expect(lintTools(tools)).toEqual([]);
|
||||
});
|
||||
|
||||
it("replaces only the schema named, leaving the sibling untouched", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: { outputSchema: { type: "object", properties: { data: true } } },
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const echo = tools.find((t) => t.name === "echo");
|
||||
// Only `outputSchema` was overridden, so the Zod-built input survives.
|
||||
expect(echo?.inputSchema).toMatchObject({ type: "object" });
|
||||
expect(
|
||||
(echo?.inputSchema as { properties?: Record<string, unknown> })
|
||||
.properties,
|
||||
).toHaveProperty("message");
|
||||
});
|
||||
|
||||
it("ignores a name that is not registered", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: { not_a_tool: { inputSchema: true } },
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
expect(tools.map((t) => t.name)).toEqual(["echo", "get_weather"]);
|
||||
expect(lintTools(tools)).toEqual([]);
|
||||
});
|
||||
|
||||
it("still calls the tool when only the input schema was overridden", async () => {
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: {
|
||||
inputSchema: {
|
||||
type: "object",
|
||||
properties: { message: { type: "string" }, opts: {} },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
// The handler and its real Zod validation are untouched by the override —
|
||||
// the fixture would be misleading if the tool it flags could not be run.
|
||||
const { tools } = await connected.listAllTools();
|
||||
const echo = tools.find((t) => t.name === "echo")!;
|
||||
const result = await connected.callTool(echo, { message: "hi" });
|
||||
expect(JSON.stringify(result)).toContain("hi");
|
||||
});
|
||||
|
||||
it("makes the call fail when an outputSchema is put on a tool with no structured content", async () => {
|
||||
// Pinning the sharp edge documented on `ServerConfig.rawToolSchemas`: a
|
||||
// conforming client validates results against the advertised output
|
||||
// schema, so an override on a preset that returns none breaks every call.
|
||||
// This is why the showcase config puts its bare-`true` outputSchema on
|
||||
// `get_temp` (which does return structured content) rather than on `echo`.
|
||||
const started = await start({
|
||||
rawToolSchemas: {
|
||||
echo: { outputSchema: { type: "object", properties: { data: true } } },
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const echo = tools.find((t) => t.name === "echo")!;
|
||||
await expect(connected.callTool(echo, { message: "hi" })).rejects.toThrow(
|
||||
/returned no structured content/,
|
||||
);
|
||||
});
|
||||
|
||||
it("serves the shape the showcase config declares", async () => {
|
||||
// Covers the JSON → ConfigFile → ServerConfig plumbing, not just the
|
||||
// in-process option: a config file is how the manual repro and the
|
||||
// screenshots in #1005 are produced, and `resolveConfig` forwarding
|
||||
// `rawToolSchemas` is a line nothing else exercises. Without this, the
|
||||
// shipped fixture could silently lose its overrides while every other
|
||||
// test in this file stayed green.
|
||||
const configPath = path.resolve(
|
||||
path.dirname(fileURLToPath(import.meta.url)),
|
||||
"../../../../../../test-servers/configs/unportable-schemas-http.json",
|
||||
);
|
||||
const resolved = resolveConfig(loadConfig(configPath));
|
||||
expect(Object.keys(resolved.rawToolSchemas ?? {}).sort()).toEqual([
|
||||
"add",
|
||||
"echo",
|
||||
"get_temp",
|
||||
]);
|
||||
|
||||
// Let the harness pick the port instead of the config's fixed one, so this
|
||||
// test can't collide with a manually-running showcase server.
|
||||
const started = await start({
|
||||
tools: resolved.tools,
|
||||
rawToolSchemas: resolved.rawToolSchemas,
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
// The exact verdict the README and the PR screenshots claim: one error on
|
||||
// get_temp, warnings on echo and add, get_weather clean.
|
||||
const results = lintTools(tools);
|
||||
expect(
|
||||
results.map((r) => ({
|
||||
tool: r.toolName,
|
||||
rules: r.findings.map((f) => f.rule).sort(),
|
||||
})),
|
||||
).toEqual([
|
||||
{ tool: "get_temp", rules: ["boolean-schema"] },
|
||||
{ tool: "echo", rules: ["type-union", "untyped-schema"] },
|
||||
{ tool: "add", rules: ["remote-ref"] },
|
||||
]);
|
||||
expect(countFindings(results)).toEqual({ errors: 1, warnings: 3 });
|
||||
|
||||
// Every flagged tool in the fixture must stay runnable — the README and
|
||||
// the PR say so, and a showcase that errors on its own obvious button is
|
||||
// worse than no showcase. Each is called with the arguments its ADVERTISED
|
||||
// schema renders, including the properties the override added, since that
|
||||
// is what a user clicking through the form would send.
|
||||
const byName = (n: string) => tools.find((t) => t.name === n)!;
|
||||
await expect(
|
||||
connected.callTool(byName("get_temp"), { city: "Oslo", units: "C" }),
|
||||
).resolves.toBeDefined();
|
||||
await expect(
|
||||
connected.callTool(byName("echo"), {
|
||||
message: "hi",
|
||||
show_ids: null,
|
||||
opts: {},
|
||||
}),
|
||||
).resolves.toBeDefined();
|
||||
await expect(
|
||||
connected.callTool(byName("add"), { a: 1, b: 2 }),
|
||||
).resolves.toBeDefined();
|
||||
});
|
||||
|
||||
it("keeps a structured-content tool callable under an outputSchema override", async () => {
|
||||
// The shape the showcase config actually ships: the bare `true` rides a
|
||||
// tool that returns structured content, and the override stays permissive
|
||||
// enough (no `required`) that the real payload still validates — so the
|
||||
// flagged tool is still runnable from the Tools tab.
|
||||
const started = await start({
|
||||
tools: [createGetTempTool(), createEchoTool()],
|
||||
rawToolSchemas: {
|
||||
get_temp: {
|
||||
outputSchema: {
|
||||
type: "object",
|
||||
properties: { data: true, city: { type: "string" } },
|
||||
},
|
||||
},
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const getTemp = tools.find((t) => t.name === "get_temp")!;
|
||||
expect(lintTools(tools).map((r) => r.toolName)).toEqual(["get_temp"]);
|
||||
|
||||
const result = await connected.callTool(getTemp, {
|
||||
city: "Oslo",
|
||||
units: "C",
|
||||
});
|
||||
expect(JSON.stringify(result)).toContain("Oslo");
|
||||
});
|
||||
|
||||
it("applies before duplication, so both copies carry the raw schema", async () => {
|
||||
const started = await start({
|
||||
duplicateToolNames: ["echo"],
|
||||
rawToolSchemas: {
|
||||
echo: { outputSchema: { type: "object", properties: { data: true } } },
|
||||
},
|
||||
});
|
||||
const connected = await connect(started.url);
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
const echoes = tools.filter((t) => t.name === "echo");
|
||||
expect(echoes).toHaveLength(2);
|
||||
for (const e of echoes) {
|
||||
expect(e.outputSchema).toEqual({
|
||||
type: "object",
|
||||
properties: { data: true },
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue