fix: rename the strict exit code, cover the config path, refresh the docs
Copilot review round 3. - Rename the `--strict` failure from "invalid" to "unportable" (`EXIT_CODES.SCHEMA_UNPORTABLE`, envelope `schema_unportable`, and the message text). The module argues at length that these schemas ARE valid JSON Schema and are merely refused by some clients, so a machine-readable code of `schema_invalid` told automated callers the opposite. Done now, while nothing depends on it. - Add a fixture-path integration case. Every other test built a `ServerConfig` by hand, so `resolveConfig`'s `rawToolSchemas` forwarding was uncovered and `unportable-schemas-http.json` was loaded by nothing — the shipped fixture could have lost its overrides with the suite still green. It now goes through loadConfig/resolveConfig and asserts the exact per-tool verdict the README claims. - Refresh the CLI README's sample output, which still showed the pre-round-2 wording and fixture, and so dropped the contract warning that rewording existed to add. Captured from a real run rather than written by hand, plus a paragraph making the suggestion pattern explicit. - Drop the self-referential "[Try it](#unportable-tool-schemas)" link in the root README, which pointed back at its own section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
cac97c142c
commit
a3d864ed6e
6 changed files with 78 additions and 13 deletions
|
|
@ -304,8 +304,8 @@ mishandled by real MCP clients:
|
|||
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.
|
||||
returns structured content) rather than `echo` — see the ⚠️ caveat at the end
|
||||
of this section for why that placement matters.
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -165,14 +165,21 @@ mcp-inspector --cli <server> --method tools/list --strict
|
|||
```
|
||||
|
||||
```text
|
||||
Error: tool "info"
|
||||
Error: tool "get_temp"
|
||||
Path: outputSchema.properties.data
|
||||
Issue: Bare `true` used where a schema object is expected.
|
||||
Suggestion: Spell the "anything" schema out — `{"type": "object", "additionalProperties": true}` …
|
||||
Suggestion: Declare what the value actually is — e.g. `{"type": "object", "additionalProperties": true}` for a free-form object. That narrows the schema deliberately; `true` accepts any JSON value at all.
|
||||
|
||||
1 error, 3 warnings across 2 tools.
|
||||
1 error, 3 warnings across 3 tools.
|
||||
```
|
||||
|
||||
Note the shape of that suggestion. Where a replacement genuinely narrows the
|
||||
schema it says so, rather than implying an equivalent rewrite — and where an
|
||||
equivalent exists it gives that instead: an array-form `type` becomes `anyOf`
|
||||
branches (not "drop it from `required`", since absent is not the same as
|
||||
`null`), and a bare `false` becomes `{"not": {}}` (not "delete the entry",
|
||||
which would *permit* the property under the default `additionalProperties`).
|
||||
|
||||
Severity decides the exit code, not the report: **errors** are constructs a
|
||||
shipping MCP client refuses outright (a bare `true` or `false` where a schema
|
||||
object belongs), **warnings** are ones handled unevenly (an array-form `type`,
|
||||
|
|
@ -302,7 +309,7 @@ prose from stderr:
|
|||
| `3` | Server requires authentication (401/403, `WWW-Authenticate`, OAuth). |
|
||||
| `4` | Server unreachable (DNS, connection refused, timeout, `fetch failed`). |
|
||||
| `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). |
|
||||
| `6` | `--strict` found an error-severity tool-schema portability problem. |
|
||||
| `6` | `--strict` found an error-severity tool-schema portability problem (`schema_unportable` — the schema is valid JSON Schema, just not portable). |
|
||||
|
||||
On any non-zero exit the CLI also writes a single JSON line to **stderr** — the
|
||||
`ErrorEnvelope`:
|
||||
|
|
|
|||
|
|
@ -128,8 +128,11 @@ describe("emitResult — schema lint wiring (#1005)", () => {
|
|||
});
|
||||
await expect(promise).rejects.toBeInstanceOf(CliExitCodeError);
|
||||
await promise.catch((e: CliExitCodeError) => {
|
||||
expect(e.exitCode).toBe(EXIT_CODES.SCHEMA_INVALID);
|
||||
expect(e.envelope?.code).toBe("schema_invalid");
|
||||
expect(e.exitCode).toBe(EXIT_CODES.SCHEMA_UNPORTABLE);
|
||||
// Not `schema_invalid`: the lint reports schemas that ARE valid JSON
|
||||
// Schema and are merely unportable, so the code an automated caller
|
||||
// branches on must not claim otherwise.
|
||||
expect(e.envelope?.code).toBe("schema_unportable");
|
||||
});
|
||||
// The result still goes to stdout; only the report is on stderr.
|
||||
expect(JSON.parse(stdout)).toEqual(DIRTY_LIST);
|
||||
|
|
|
|||
|
|
@ -9,6 +9,11 @@
|
|||
* - 4: server unreachable (DNS, connect refused, timeout, fetch failure)
|
||||
* - 5: tool error (`tools/call` returned `isError:true`, or tool not found)
|
||||
* - 6: `--strict` found an error-severity tool-schema portability finding
|
||||
*
|
||||
* Note 6 is `SCHEMA_UNPORTABLE`, not "invalid": the whole premise of the lint
|
||||
* is that these schemas ARE valid JSON Schema and are merely refused by some
|
||||
* clients. Calling the outcome "invalid" would misreport it to the automated
|
||||
* callers this map exists for.
|
||||
*/
|
||||
export const EXIT_CODES = {
|
||||
OK: 0,
|
||||
|
|
@ -17,7 +22,7 @@ export const EXIT_CODES = {
|
|||
AUTH_REQUIRED: 3,
|
||||
UNREACHABLE: 4,
|
||||
TOOL_ERROR: 5,
|
||||
SCHEMA_INVALID: 6,
|
||||
SCHEMA_UNPORTABLE: 6,
|
||||
} as const;
|
||||
|
||||
/** Machine-readable error envelope written as one JSON line on stderr. */
|
||||
|
|
|
|||
|
|
@ -62,10 +62,13 @@ export async function emitResult(
|
|||
if (args.strict && lint) {
|
||||
const { errors } = countFindings(lint);
|
||||
if (errors > 0) {
|
||||
// "unportable", not "invalid": these schemas are valid JSON Schema and
|
||||
// are merely refused by some clients, so an envelope code of
|
||||
// `schema_invalid` would tell an automated caller the wrong thing.
|
||||
throw new CliExitCodeError(
|
||||
EXIT_CODES.SCHEMA_INVALID,
|
||||
`${errors} tool schema error${errors === 1 ? "" : "s"} found (--strict).`,
|
||||
{ code: "schema_invalid" },
|
||||
EXIT_CODES.SCHEMA_UNPORTABLE,
|
||||
`${errors} tool schema portability error${errors === 1 ? "" : "s"} found (--strict).`,
|
||||
{ code: "schema_unportable" },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,9 @@
|
|||
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 { lintTools } from "@inspector/core/json/schemaLint.js";
|
||||
import { countFindings, lintTools } from "@inspector/core/json/schemaLint.js";
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
|
|
@ -9,6 +11,8 @@ import {
|
|||
createEchoTool,
|
||||
createGetWeatherTool,
|
||||
createGetTempTool,
|
||||
loadConfig,
|
||||
resolveConfig,
|
||||
} from "@modelcontextprotocol/inspector-test-server";
|
||||
import type { ServerConfig } from "@modelcontextprotocol/inspector-test-server";
|
||||
|
||||
|
|
@ -211,6 +215,49 @@ describe("rawToolSchemas override (#1005)", () => {
|
|||
);
|
||||
});
|
||||
|
||||
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 });
|
||||
});
|
||||
|
||||
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
|
||||
|
|
|
|||
Loading…
Reference in a new issue