fix: address Copilot review on the schema portability lint
- Scope `non-object-root` to `inputSchema`. The pinned SDK types
`inputSchema` with `type: literal("object")` but `outputSchema` as a
bare `looseObject`, so a `{"type":"string"}` output is conforming; the
rule was claiming a requirement MCP does not make, at error severity.
- Decide `untyped-schema` from an allowlist of constraining keywords
rather than a denylist of annotations. JSON Schema ignores keywords it
does not recognize, so `{"vendorHint": true}` accepts every value and a
denylist passed it as constrained. `$defs`-only schemas now flag too.
- Walk `dependentSchemas` and draft-07 `dependencies`, with a test
pinning that the property-name-array form is not a schema position.
- Suggest `anyOf` branches for an array-form `type`, never "drop it from
`required`" — accepting `null` and permitting absence are independent
contracts, so the old suggestion silently changed the schema's meaning.
- Reject `--strict` alongside `--app-info`. That path returns NDJSON from
`runMethod` and never reaches the lint, so the flag was a no-op gate
that could never fail.
- Colour the web sidebar flag from the `--inspector-*` severity tokens,
matching the detail panel and avoiding the default yellow that failed
the a11y check.
- Add a live integration test for `rawToolSchemas`. It caught that an
`outputSchema` override on a preset returning no structured content
makes every call to that tool fail client-side validation — the
showcase config did exactly that, so the flagged tool could not be
run. Moved the bare `true` onto `get_temp`, documented the caveat, and
pinned both halves.
- Drop the double cast from the lint test fixture.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
2f9e0901c1
commit
c389565783
9 changed files with 566 additions and 66 deletions
35
README.md
35
README.md
|
|
@ -290,15 +290,22 @@ The **TUI** had the same gap and is worth checking against the same server (`--t
|
|||
|
||||
#### Unportable tool schemas
|
||||
|
||||
`unportable-schemas-http.json` serves three tools, two of whose advertised
|
||||
`unportable-schemas-http.json` serves four tools, three of whose advertised
|
||||
schemas carry constructs that are legal JSON Schema and are refused or
|
||||
mishandled by real MCP clients: `echo` declares `outputSchema.properties.data`
|
||||
as a bare `true` (what Go's `jsonschema` package emits for `interface{}` —
|
||||
the case reported in [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005)),
|
||||
an array-form `"type": ["null","boolean"]`, and an `opts` property that
|
||||
constrains nothing; `add` points a property at a remote `$ref`. `get_temp` is
|
||||
left clean, so a flagged tool sits next to an unflagged one. Plain
|
||||
streamable-HTTP — connect with the **default (legacy)** protocol era.
|
||||
mishandled by real MCP clients:
|
||||
|
||||
| Tool | What it carries |
|
||||
| --- | --- |
|
||||
| `get_temp` | `outputSchema.properties.data` as a bare `true` — what Go's `jsonschema` package emits for `interface{}`, the case reported in [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005) |
|
||||
| `echo` | an array-form `"type": ["null","boolean"]`, and an `opts` property that constrains nothing |
|
||||
| `add` | a property pointing at a remote `$ref` |
|
||||
| `get_weather` | nothing — left clean, so a flagged tool sits beside an unflagged one |
|
||||
|
||||
Plain streamable-HTTP — connect with the **default (legacy)** protocol era.
|
||||
Every tool here is still **runnable**: the override replaces only the
|
||||
*advertised* schema, and the flagship bare-`true` rides `get_temp` (which
|
||||
returns structured content) rather than `echo` — see the caveat under
|
||||
[Try it](#unportable-tool-schemas) below.
|
||||
|
||||
The Inspector is where a server author looks first, so a construct that will
|
||||
fail downstream is named here rather than passed through silently. All three
|
||||
|
|
@ -313,8 +320,9 @@ mcp-inspector --cli http://127.0.0.1:6603/mcp --method tools/list --strict # e
|
|||
- **CLI** — `--strict` prints the full report (path, issue, suggested fix) on
|
||||
stderr and exits `6` on an error-severity finding; without it, one summary
|
||||
line. See [Schema portability](./clients/cli/README.md#schema-portability---strict).
|
||||
- **TUI** — the tools list marks `echo` with a red `!` and `add` with a yellow
|
||||
`?`; the detail pane lists each finding under **Schema Portability**.
|
||||
- **TUI** — the tools list marks `get_temp` with a red `!` and `echo`/`add`
|
||||
with a yellow `?`; the detail pane lists each finding under **Schema
|
||||
Portability**.
|
||||
- **Web** — the Tools sidebar row carries the same flag as a hover-labelled
|
||||
icon, and selecting the tool shows a **Schema portability** section above the
|
||||
argument form.
|
||||
|
|
@ -329,6 +337,13 @@ test server's `rawToolSchemas` override, because the Zod-built presets cannot
|
|||
express any of them — which is the same reason a real server hits this only
|
||||
when its schemas come from another generator.
|
||||
|
||||
⚠️ **If you add an `outputSchema` override of your own, put it on a tool that
|
||||
returns structured content.** A conforming client validates a tool result
|
||||
against the advertised output schema, so an override on a preset that returns
|
||||
none makes every call to it fail with "declares an output schema but returned
|
||||
no structured content" — a confusing thing to hit from a fixture. That is why
|
||||
the bare `true` rides `get_temp` here and not `echo`.
|
||||
|
||||
#### RFC 6570 resource templates
|
||||
|
||||
`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era.
|
||||
|
|
|
|||
|
|
@ -207,6 +207,9 @@ describe("emitResult — schema lint wiring (#1005)", () => {
|
|||
});
|
||||
|
||||
it("does not lint an --app-info run", async () => {
|
||||
// `runCli` rejects `--strict --app-info` outright (see below), so this is
|
||||
// the defensive half: any other Node runner reusing `emitResult` gets the
|
||||
// app-info early return rather than a report interleaved with its NDJSON.
|
||||
await emitResult(
|
||||
DIRTY_LIST,
|
||||
{ hasApp: true, toolName: "info" },
|
||||
|
|
@ -248,4 +251,23 @@ describe("--strict argument validation", () => {
|
|||
]),
|
||||
).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
|
||||
// a `--strict` gate that can never fail.
|
||||
await expect(
|
||||
runCli([
|
||||
"node",
|
||||
"cli",
|
||||
"--cli",
|
||||
"--method",
|
||||
"tools/list",
|
||||
"--strict",
|
||||
"--app-info",
|
||||
"--server-url",
|
||||
"http://127.0.0.1:1/mcp",
|
||||
]),
|
||||
).rejects.toThrow("--strict cannot be combined with --app-info");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1013,6 +1013,17 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
|
|||
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.",
|
||||
);
|
||||
}
|
||||
|
||||
// --tool-args-json passes arguments verbatim with no key=value coercion (so
|
||||
// `"012"` stays a string and nested objects work without shell escaping).
|
||||
let toolArg = options.toolArg;
|
||||
|
|
|
|||
|
|
@ -57,6 +57,11 @@ const ListItemButton = UnstyledButton.withProps({
|
|||
// 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",
|
||||
|
|
@ -86,7 +91,11 @@ export function ToolListItem({ tool, selected, onClick }: ToolListItemProps) {
|
|||
withArrow
|
||||
>
|
||||
<SchemaFlagIcon
|
||||
color={hasError ? "red" : "yellow"}
|
||||
c={
|
||||
hasError
|
||||
? "var(--inspector-danger-text)"
|
||||
: "var(--inspector-warning-text)"
|
||||
}
|
||||
aria-label={
|
||||
hasError
|
||||
? "Schema portability errors"
|
||||
|
|
|
|||
|
|
@ -107,8 +107,11 @@ describe("lintToolSchemas — clean schemas", () => {
|
|||
});
|
||||
|
||||
it("reports nothing when a tool declares neither schema", () => {
|
||||
const bare = { name: "t" } as unknown as Tool;
|
||||
expect(lintToolSchemas(bare)).toEqual([]);
|
||||
expect(
|
||||
lintToolSchemas(
|
||||
tool({ inputSchema: undefined, outputSchema: undefined }),
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -174,6 +177,16 @@ describe("lintToolSchemas — boolean-schema", () => {
|
|||
{ type: "object", propertyNames: true, properties: {} },
|
||||
"inputSchema.propertyNames",
|
||||
],
|
||||
[
|
||||
"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 }));
|
||||
|
|
@ -192,7 +205,7 @@ describe("lintToolSchemas — boolean-schema", () => {
|
|||
});
|
||||
|
||||
describe("lintToolSchemas — type-union", () => {
|
||||
it("flags the nullable array form and suggests the single type", () => {
|
||||
it("flags the nullable array form and suggests anyOf branches", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
|
|
@ -203,10 +216,27 @@ describe("lintToolSchemas — type-union", () => {
|
|||
);
|
||||
expect(rules(findings)).toEqual(["type-union"]);
|
||||
expect(findings[0]!.severity).toBe("warning");
|
||||
expect(findings[0]!.suggestion).toContain('"type": "boolean"');
|
||||
expect(findings[0]!.suggestion).toContain(
|
||||
'{"anyOf": [{"type": "null"}, {"type": "boolean"}]}',
|
||||
);
|
||||
});
|
||||
|
||||
it("suggests anyOf when more than one non-null member survives", () => {
|
||||
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: {
|
||||
|
|
@ -215,10 +245,12 @@ describe("lintToolSchemas — type-union", () => {
|
|||
},
|
||||
}),
|
||||
);
|
||||
expect(findings[0]!.suggestion).toContain("anyOf");
|
||||
expect(findings[0]!.suggestion).toContain(
|
||||
'{"anyOf": [{"type": "string"}, {"type": "number"}]}',
|
||||
);
|
||||
});
|
||||
|
||||
it("ignores non-string members when picking the suggestion", () => {
|
||||
it("drops non-string members from the suggested branches", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: {
|
||||
|
|
@ -227,7 +259,19 @@ describe("lintToolSchemas — type-union", () => {
|
|||
},
|
||||
}),
|
||||
);
|
||||
expect(findings[0]!.suggestion).toContain('"type": "string"');
|
||||
expect(findings[0]!.suggestion).toContain(
|
||||
'{"anyOf": [{"type": "string"}]}',
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to a placeholder when no member is a string", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({
|
||||
inputSchema: { type: "object", properties: { a: { type: [3] } } },
|
||||
}),
|
||||
);
|
||||
expect(rules(findings)).toEqual(["type-union"]);
|
||||
expect(findings[0]!.suggestion).toContain("anyOf");
|
||||
});
|
||||
|
||||
it("does not raise non-object-root for an array-typed root", () => {
|
||||
|
|
@ -258,6 +302,51 @@ describe("lintToolSchemas — 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([
|
||||
["pattern", { pattern: "^a" }],
|
||||
["format", { format: "email" }],
|
||||
["required", { required: ["a"] }],
|
||||
["minimum", { minimum: 0 }],
|
||||
["contains", { contains: { type: "string" } }],
|
||||
["propertyNames", { propertyNames: { pattern: "^a" } }],
|
||||
["dependentRequired", { dependentRequired: { a: ["b"] } }],
|
||||
])("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"]);
|
||||
|
|
@ -277,12 +366,14 @@ describe("lintToolSchemas — non-object-root", () => {
|
|||
expect(findings[0]!.issue).toContain("inputSchema");
|
||||
});
|
||||
|
||||
it("names outputSchema when that is the offending root", () => {
|
||||
const findings = lintToolSchemas(
|
||||
tool({ outputSchema: { type: "string" } }),
|
||||
it("does NOT flag a non-object outputSchema root", () => {
|
||||
// The SDK's `Tool` schema types `inputSchema` with `type: literal("object")`
|
||||
// but `outputSchema` as a plain loose object with no `type` constraint, so
|
||||
// a string-typed output is conforming. Flagging it would report an error
|
||||
// MCP does not require and would fail `--strict` on a valid server.
|
||||
expect(lintToolSchemas(tool({ outputSchema: { type: "string" } }))).toEqual(
|
||||
[],
|
||||
);
|
||||
expect(rules(findings)).toEqual(["non-object-root"]);
|
||||
expect(findings[0]!.issue).toContain("outputSchema");
|
||||
});
|
||||
|
||||
it("does not flag a nested non-object schema", () => {
|
||||
|
|
@ -332,6 +423,21 @@ describe("lintToolSchemas — walk robustness", () => {
|
|||
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({
|
||||
|
|
|
|||
244
clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts
Normal file
244
clients/web/src/test/integration/mcp/raw-tool-schemas.test.ts
Normal file
|
|
@ -0,0 +1,244 @@
|
|||
import { describe, it, expect, afterEach } from "vitest";
|
||||
import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
|
||||
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
|
||||
import { lintTools } from "@inspector/core/json/schemaLint.js";
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
createTestServerInfo,
|
||||
createEchoTool,
|
||||
createGetWeatherTool,
|
||||
createGetTempTool,
|
||||
} 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("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("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 },
|
||||
});
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
@ -103,37 +103,97 @@ const SUBSCHEMA_ARRAY_KEYWORDS = [
|
|||
"prefixItems",
|
||||
] as const;
|
||||
|
||||
/** Keywords whose value is a name → subschema map. */
|
||||
/**
|
||||
* Keywords whose value is a name → subschema map.
|
||||
*
|
||||
* `dependencies` is the pre-2019 form and may hold either a subschema or an
|
||||
* array of property names; the array case is not a schema position, and the
|
||||
* walk skips it because a non-object, non-boolean node is not a schema.
|
||||
*/
|
||||
const SUBSCHEMA_MAP_KEYWORDS = [
|
||||
"properties",
|
||||
"patternProperties",
|
||||
"dependentSchemas",
|
||||
"dependencies",
|
||||
"$defs",
|
||||
"definitions",
|
||||
] as const;
|
||||
|
||||
/**
|
||||
* Keywords that carry no validation meaning — metadata a schema may hold
|
||||
* alongside its constraints. A schema built *only* from these accepts every
|
||||
* value, which is what the `untyped-schema` rule reports.
|
||||
* Keywords that actually constrain the instance being validated — the
|
||||
* assertion and applicator vocabularies, plus reference keywords.
|
||||
*
|
||||
* Defining the rule this way, rather than as "has no `type`", is deliberate:
|
||||
* `{"properties": {…}}` with no `type` does constrain its input, so calling it
|
||||
* unconstrained would be false. That shape is left alone; the rule fires on
|
||||
* `{}` and on annotation-only schemas — the object-literal spelling of the
|
||||
* same "any" that a bare `true` expresses.
|
||||
* This is an **allowlist**, and that direction is load-bearing. JSON Schema
|
||||
* ignores keywords it does not recognize, so a schema is unconstrained unless
|
||||
* something it carries is known to constrain: a denylist of known-annotation
|
||||
* keywords would let `{"vendorHint": true}` — which accepts every value —
|
||||
* pass as constrained simply because the keyword is unfamiliar.
|
||||
*
|
||||
* Two categories are deliberately absent. **Annotations** (`title`,
|
||||
* `description`, `default`, `examples`, `deprecated`, `readOnly`,
|
||||
* `writeOnly`, `$comment`) assert nothing. So do the **definition-only
|
||||
* containers** `$defs` / `definitions` and the identifier keywords `$id`,
|
||||
* `$schema`, `$anchor`, `$vocabulary`: they hold or name subschemas for
|
||||
* *other* schemas to reference, and constrain the current instance not at all.
|
||||
*
|
||||
* `format` is included even though 2020-12's default vocabulary treats it as
|
||||
* an annotation rather than an assertion. A schema declaring one is plainly an
|
||||
* attempt to constrain, and reporting it as accepting anything would be noise
|
||||
* rather than a finding an author can act on.
|
||||
*
|
||||
* Defining the rule this way, rather than as "has no `type`", is also
|
||||
* deliberate: `{"properties": {…}}` with no `type` does constrain its input,
|
||||
* so calling it unconstrained would be false. The rule fires on `{}`, on
|
||||
* annotation-only schemas, and on `$defs`-only schemas — the object-literal
|
||||
* spellings of the same "any" that a bare `true` expresses.
|
||||
*/
|
||||
const ANNOTATION_ONLY_KEYWORDS: ReadonlySet<string> = new Set([
|
||||
"title",
|
||||
"description",
|
||||
"default",
|
||||
"examples",
|
||||
"deprecated",
|
||||
"readOnly",
|
||||
"writeOnly",
|
||||
"$comment",
|
||||
"$id",
|
||||
"$schema",
|
||||
"$anchor",
|
||||
const CONSTRAINING_KEYWORDS: ReadonlySet<string> = new Set([
|
||||
// Assertions
|
||||
"type",
|
||||
"enum",
|
||||
"const",
|
||||
"multipleOf",
|
||||
"maximum",
|
||||
"exclusiveMaximum",
|
||||
"minimum",
|
||||
"exclusiveMinimum",
|
||||
"maxLength",
|
||||
"minLength",
|
||||
"pattern",
|
||||
"format",
|
||||
"maxItems",
|
||||
"minItems",
|
||||
"uniqueItems",
|
||||
"maxContains",
|
||||
"minContains",
|
||||
"maxProperties",
|
||||
"minProperties",
|
||||
"required",
|
||||
"dependentRequired",
|
||||
// Applicators
|
||||
"allOf",
|
||||
"anyOf",
|
||||
"oneOf",
|
||||
"not",
|
||||
"if",
|
||||
"then",
|
||||
"else",
|
||||
"items",
|
||||
"prefixItems",
|
||||
"contains",
|
||||
"additionalItems",
|
||||
"unevaluatedItems",
|
||||
"properties",
|
||||
"patternProperties",
|
||||
"additionalProperties",
|
||||
"unevaluatedProperties",
|
||||
"propertyNames",
|
||||
"dependentSchemas",
|
||||
"dependencies",
|
||||
// References
|
||||
"$ref",
|
||||
"$dynamicRef",
|
||||
"$recursiveRef",
|
||||
]);
|
||||
|
||||
/**
|
||||
|
|
@ -263,18 +323,26 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void {
|
|||
const type = node.type;
|
||||
|
||||
if (Array.isArray(type)) {
|
||||
const nonNull = type.filter(
|
||||
(t): t is string => typeof t === "string" && t !== "null",
|
||||
);
|
||||
const members = type.filter((t): t is string => typeof t === "string");
|
||||
// The suggestion is always `anyOf`, never "drop it from `required`".
|
||||
// Accepting an explicit JSON `null` and permitting the property to be
|
||||
// absent are independent: on a required `["null","boolean"]` field,
|
||||
// un-requiring it would both stop accepting `null` and start accepting
|
||||
// omission — a different contract, not the same one spelled portably.
|
||||
// `anyOf` branches each carrying a single `type` are equivalent, and this
|
||||
// lint treats them as portable.
|
||||
const branches = members.length > 0 ? members : type.map(() => "…");
|
||||
add(
|
||||
ctx,
|
||||
"type-union",
|
||||
"warning",
|
||||
path,
|
||||
`\`type\` is an array (${JSON.stringify(type)}). The array form is legal JSON Schema, but several MCP clients read \`type\` as a single string and either reject the tool or drop the constraint.`,
|
||||
nonNull.length === 1
|
||||
? `Use \`"type": "${nonNull[0]}"\` and leave the property out of \`required\` to express "may be absent".`
|
||||
: "Split the alternatives into `anyOf` branches, each with a single `type`.",
|
||||
`Split it into \`anyOf\` branches, each with a single \`type\` — \`{"anyOf": [${branches
|
||||
.map((t) => `{"type": "${t}"}`)
|
||||
.join(
|
||||
", ",
|
||||
)}]}\`. (Making the property optional instead is a different contract: absent is not the same as \`null\`.)`,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -290,8 +358,8 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void {
|
|||
);
|
||||
}
|
||||
|
||||
const constrains = Object.keys(node).some(
|
||||
(key) => !ANNOTATION_ONLY_KEYWORDS.has(key),
|
||||
const constrains = Object.keys(node).some((key) =>
|
||||
CONSTRAINING_KEYWORDS.has(key),
|
||||
);
|
||||
if (!constrains) {
|
||||
add(
|
||||
|
|
@ -306,9 +374,18 @@ function lintNode(node: SchemaRecord, path: string, ctx: WalkContext): void {
|
|||
}
|
||||
|
||||
/**
|
||||
* Lint one of a tool's schemas. The root gets one extra rule: MCP requires a
|
||||
* tool schema to describe an object, and a root of any other type is refused
|
||||
* by the SDK-side parsers rather than merely degraded.
|
||||
* Lint one of a tool's schemas.
|
||||
*
|
||||
* `inputSchema` gets one extra rule at the root: MCP requires it to describe
|
||||
* an object, and the SDK enforces that — its `Tool` schema types `inputSchema`
|
||||
* with `type: literal("object")`, so a non-object root is refused by the
|
||||
* parser rather than merely degraded.
|
||||
*
|
||||
* That rule is **scoped to `inputSchema` deliberately**. The same SDK types
|
||||
* `outputSchema` as a plain loose object with no `type` constraint at all, so
|
||||
* a tool whose output is `{"type": "string"}` is conforming; flagging it would
|
||||
* report an error MCP does not require and would fail `--strict` on a valid
|
||||
* server.
|
||||
*/
|
||||
function lintSchema(
|
||||
schema: unknown,
|
||||
|
|
@ -316,7 +393,7 @@ function lintSchema(
|
|||
findings: SchemaFinding[],
|
||||
): void {
|
||||
const ctx: WalkContext = { schema: kind, findings };
|
||||
if (isRecord(schema)) {
|
||||
if (kind === "inputSchema" && isRecord(schema)) {
|
||||
const type = schema.type;
|
||||
if (typeof type === "string" && type !== "object") {
|
||||
add(
|
||||
|
|
@ -324,7 +401,7 @@ function lintSchema(
|
|||
"non-object-root",
|
||||
"error",
|
||||
"",
|
||||
`Root \`type\` is "${type}". MCP requires a tool's ${kind} to describe an object.`,
|
||||
`Root \`type\` is "${type}". MCP requires a tool's inputSchema to describe an object.`,
|
||||
'Wrap the value in an object schema — `{"type": "object", "properties": {…}}`.',
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,8 +3,22 @@
|
|||
"name": "unportable-schemas-showcase",
|
||||
"version": "1.0.0"
|
||||
},
|
||||
"tools": [{ "preset": "echo" }, { "preset": "add" }, { "preset": "get_temp" }],
|
||||
"tools": [
|
||||
{ "preset": "get_temp" },
|
||||
{ "preset": "echo" },
|
||||
{ "preset": "add" },
|
||||
{ "preset": "get_weather" }
|
||||
],
|
||||
"rawToolSchemas": {
|
||||
"get_temp": {
|
||||
"outputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"data": true,
|
||||
"city": { "type": "string" }
|
||||
}
|
||||
}
|
||||
},
|
||||
"echo": {
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
|
|
@ -13,13 +27,6 @@
|
|||
"show_ids": { "type": ["null", "boolean"] },
|
||||
"opts": {}
|
||||
}
|
||||
},
|
||||
"outputSchema": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"data": true,
|
||||
"topic": { "type": "string" }
|
||||
}
|
||||
}
|
||||
},
|
||||
"add": {
|
||||
|
|
|
|||
|
|
@ -507,6 +507,15 @@ export interface ServerConfig {
|
|||
* *advertised* schema is replaced; the tool's handler and its real Zod
|
||||
* validation are untouched, so calling it still behaves as its preset does.
|
||||
*
|
||||
* ⚠️ **Overriding `outputSchema` changes what a client will accept.** A
|
||||
* conforming client validates a tool result against the advertised output
|
||||
* schema, so putting one on a preset whose handler returns no
|
||||
* `structuredContent` makes every call to it fail — the Inspector rejects it
|
||||
* with "declares an output schema but returned no structured content". Put an
|
||||
* `outputSchema` override only on a tool that actually returns structured
|
||||
* content (`get_temp`, `list_items`), and keep the override permissive enough
|
||||
* that the real payload still validates.
|
||||
*
|
||||
* A name that isn't registered is ignored.
|
||||
*/
|
||||
rawToolSchemas?: Record<
|
||||
|
|
|
|||
Loading…
Reference in a new issue