feat: flag unportable tool schemas in all three clients

[cliffhall]

Closes#1005

What

A tool's inputSchema/outputSchema can be perfectly legal JSON Schema and still be rejected outright by the client the server is meant to run against. The reported case is Go's jsonschema package emitting a bare true for an interface{} field: valid Draft 2020-12, accepted by the MCP SDK's own parser, passed through by the Inspector without a word — and refused by Claude Code with an opaque "Invalid input" pointing at ["tools", 7, "outputSchema", "properties", "data"].

This adds a tool-schema portability lint — core/json/schemaLint.ts — and surfaces one shared verdict in all three clients.

The issue was filed against the CLI (and, in January, against v1), but the gap is not CLI-shaped: the Inspector is where a server author looks first, whichever client they happen to be in. So the module is pure and shared, and each client reports it with the room it has.

The rules, and why it is not a validator

Deliberately not a JSON Schema validator, for a reason measured on the issue itself: a census of 617 public servers (14,804 tool schemas) found 0 that fail the SDK's own ListToolsResultSchema.safeParse. A conformance check would report nothing on essentially every real server. What bites is the narrower subset each consumer accepts, so every rule here is a construct that is legal JSON Schema and known to be refused or quietly mishandled by a shipping client.

Rule	Severity	Fires on
boolean-schema	error	A bare true/false where a schema object is expected — the Go interface{} case
type-union	warning	The array form, "type": ["null","boolean"]
remote-ref	warning	A $ref pointing outside the document
untyped-schema	warning	A schema carrying no constraining keyword at all — the object-literal spelling of true

Five precision decisions worth reviewing:

• additionalProperties: true is not flagged. A boolean is the idiomatic spelling under additionalProperties / unevaluatedProperties / additionalItems / unevaluatedItems, and it is the issue's own suggested fix. The rule fires only in positions where a boolean is legal-but-unportable.
• There is no "inputSchema must be an object" rule, even though MCP requires one. The SDK types inputSchema with type: literal("object"), so such a tool fails ListToolsResultSchema and salvageListItems drops it before any client sees it — verified against a live server, where listAllTools() returns []. A rule that cannot fire is worse than none, because the docs then claim a check the tool does not perform. The condition is already reported through the malformed-items surface.
• untyped-schema uses an allowlist of constraining keywords, not a denylist of annotations. JSON Schema ignores keywords it does not recognize, so {"vendorHint": true} accepts every value; a denylist would pass it as constrained merely because the keyword is unfamiliar. $defs-only schemas are caught for the same reason. Conversely {"properties": {…}} with no typedoes constrain its input, so it is left alone — as is {} under not, where an always-accepting subschema means always-reject.
• Every suggestion says whether it preserves the contract.false in a property position forbids that property, so the fix is {"not": {}} and not "delete the entry" (which would permit it under the default additionalProperties). An array-form type becomes anyOf branches, not "drop it from required" (absent ≠ null). Where a replacement genuinely narrows — true, and the untyped case — the text says so rather than implying equivalence.
• Each rule stays quiet on schemas that are merely malformed.type-union fires only on a non-empty, unique array of recognized type names, because its message asserts the construct is legal JSON Schema — which would be false for [], [3], or ["bananas"], and the anyOf it suggests for those would be invalid too. Same principle as the walk skipping a node that is neither an object nor a boolean: malformed is the SDK parser's business.

Surfaces

CLI — --strict (with --method tools/list) prints the report from the issue on stderr and exits 6 if any finding is error-severity:

Error: tool "get_temp"
Path: outputSchema.properties.data
Issue: Bare `true` used where a schema object is expected.
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.
Warning: tool "echo"
Path: inputSchema.properties.show_ids
Issue: `type` is an array (["null","boolean"]). …
Suggestion: Split it into `anyOf` branches, each with a single `type` — `{"anyOf": [{"type": "null"}, {"type": "boolean"}]}`. (Making the property optional instead is a different contract: absent is not the same as `null`.)
1 error, 3 warnings across 3 tools.

Only errors fail the run — a --strict that failed on warnings would be unusable as a CI gate against servers that are in fact fine. Under --format json the findings ride the same envelope ({"result":…,"schemaFindings":[…]}) rather than as a second document. --strict is rejected alongside --app-info, because that path returns NDJSON straight from runMethod and never reaches the lint — accepting the pair would hand a CI caller a gate that can never fail. Without --strict, nothing changes but one line: a summary on stderr (Schema portability: 1 error, 3 warnings across 3 tools. Re-run with --strict for details.), still exit 0, and nothing at all on a clean list.

TUI — the tools list flags an offending tool (! red / ? yellow) and the detail pane lists each finding under Schema Portability. Rendered frame (the terminal can't be screenshotted headlessly, so this is ToolsTab's real output through ink-testing-library, colour stripped):

 Tools (4) │ get_temp [Enter to Test]
│
▶ get_temp ! │ Get the current temperature for a city
echo ? │
add ? │ Schema Portability (1):
get_weather │
│ ! outputSchema.properties.data
│ Bare `true` used where a schema object is expected.
│ Fix: 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.
│
│ Input Schema:
│
│ {
│ "type": "object",
│ …

Web — the Tools sidebar row carries a severity icon (hover-labelled, and named in the accessible label); selecting the tool renders a Schema portability section above the argument form.

Two accessibility details in there are fixes rather than preferences:

• Severity is carried by shape, not only colour — a circle for an error, a triangle for a warning — so a colour-blind reader can still tell them apart in the sidebar. The TUI does the same with ! vs ?.
• Both web surfaces colour from the app's --inspector-danger-text / --inspector-warning-text tokens rather than Mantine's color="yellow". A filled yellow badge puts white on yellow-7, which at that size is 3.92:1 and fails the story's a11y check; neither autoContrast (yellow-7 sits below Mantine's default luminance threshold, so the label stays white) nor variant="light" (3.34:1) rescues it.

Web screenshots

Tools list — get_temp flagged red (an error-severity finding), echo and add yellow (warnings only), get_weather unmarked:

Detail panel — one block per finding, above the argument form:

A warning-only tool (echo, two warnings) — same section, different severity labels and different findings, and nothing here fails --strict:

The sidebar flag's tooltip carries the severity breakdown rather than the bare total, so a tool with a mix is not announced as all-errors:

A clean tool (get_weather) — the section is absent entirely, which is the case for essentially every real server:

Try it

test-servers/configs/unportable-schemas-http.json exercises every rule, with get_weather left clean for contrast. It needs a new rawToolSchemas config override, because the Zod-built presets cannot emit any of these constructs — which is the same reason a real server only hits this when its schemas come from another generator.

npm run test-servers:build --prefix clients/web
node test-servers/build/server-composable.js --config test-servers/configs/unportable-schemas-http.json
mcp-inspector --cli http://127.0.0.1:6603/mcp --method tools/list --strict # exits 6

Every tool in that fixture is still runnable, and keeping that true took two deliberate choices, both now pinned by tests:

• A conforming client validates a result against the advertised output schema, so an outputSchema override on a preset returning no structuredContent makes every call to it fail. The flagship bare true therefore rides get_temp (which does return structured content) rather than echo.
• The remote-ref demo keeps a local type alongside the $ref. The TUI's schemaToForm dispatches on type, so a $ref-only property falls through to a string field that add's numeric handler rejects — the finding would have been demonstrated at the cost of the tool.

Testing

• core/json/schemaLint.ts — 78 cases: every rule, every walk position (properties, items in both forms, prefixItems, $defs, patternProperties, dependentSchemas, draft-07 dependencies in both its schema and property-list forms, combinator branches), path quoting, the depth cap, the non-schema/wrong-shape skips, and the report/summary formatters.
• rawToolSchemas — a new integration test boots the composable server over a real transport and asserts the raw document actually reaches the wire, that an unnamed tool keeps its Zod schema, that the override survives duplication, both halves of the outputSchema caveat above, and that a non-object input root is dropped by salvage before any client sees it (the fact the missing root rule rests on).
• CLI — 19 cases over the handler and the emitResult wiring: exit 6 on error, exit 0 on warnings-only, the --format json envelope, both --strict argument rejections driven through the real runCli, and isError (exit 5) winning over the schema verdict.
• Web — SchemaFindingsList unit tests + 4 Storybook play stories; ToolListItem and ToolDetailPanel cases for the flag and the section.
• TUI — list-marker and detail-pane cases, plus schemaMarker directly.
• npm run ci green.

[Copilot]

Pull request overview

Adds shared tool-schema portability linting across Web, TUI, and CLI clients.

Changes:

• Introduces shared schema lint rules and reporting helpers.
• Adds CLI --strict, Web indicators, and TUI findings.
• Adds fixtures, tests, and documentation.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
core/json/schemaLint.ts	Implements shared schema linting.
clients/cli/src/cli.ts	Adds --strict argument handling.
clients/cli/src/error-handler.ts	Adds schema-invalid exit code.
clients/cli/src/handlers/emit-result.ts	Emits findings and strict failures.
clients/cli/src/handlers/method-types.ts	Adds strict method option.
clients/cli/src/handlers/schema-lint-report.ts	Formats CLI lint reports.
clients/cli/__tests__/schema-lint-report.test.ts	Tests CLI reporting behavior.
clients/cli/README.md	Documents strict mode.
clients/tui/src/components/ToolsTab.tsx	Displays TUI markers and findings.
clients/tui/__tests__/ToolsTab.test.tsx	Tests TUI schema indicators.
clients/tui/README.md	Documents TUI behavior.
clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.tsx	Renders Web findings.
clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.test.tsx	Tests findings rendering.
clients/web/src/components/elements/SchemaFindingsList/SchemaFindingsList.stories.tsx	Adds Storybook scenarios.
clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.tsx	Adds findings to tool details.
clients/web/src/components/groups/ToolDetailPanel/ToolDetailPanel.test.tsx	Tests detail findings.
clients/web/src/components/groups/ToolListItem/ToolListItem.tsx	Adds sidebar warning icon.
clients/web/src/components/groups/ToolListItem/ToolListItem.test.tsx	Tests sidebar indicators.
clients/web/src/test/core/schemaLint.test.ts	Tests shared lint rules.
clients/web/README.md	Documents Web integration.
test-servers/src/load-config.ts	Defines raw schema configuration.
test-servers/src/resolve-config.ts	Propagates raw schema settings.
test-servers/src/composable-test-server.ts	Overrides advertised tool schemas.
test-servers/configs/unportable-schemas-http.json	Adds demonstration fixture.
README.md	Documents the feature and fixture.
AGENTS.md	Records the shared architecture.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Review round 1 — all 8 comments addressed (c3895657)

Every one was valid; three were real defects rather than polish. Mirroring here because the inline threads go outdated once the fix is pushed.

#	Finding	Outcome
1	non-object-root also fired on outputSchema	Fixed — scoped to inputSchema
2	untyped-schema used a denylist, so unknown keywords counted as validation	Fixed — allowlist of constraining keywords
3	Walk missed dependentSchemas / dependencies	Fixed — both walked, array form pinned
4	Double cast in the lint test	Fixed — fixture narrows once
5	No live test for rawToolSchemas	Fixed — integration test added, and it caught a real bug
6	Sidebar icon used the default yellow	Fixed — same severity tokens as the detail panel
7	type-union suggestion changed the schema's meaning	Fixed — suggests anyOf
8	--strict --app-info was a silent no-op	Fixed — combination rejected

The three worth reading

#1 — the SDK settles it.ToolSchema types inputSchema as z.object({ type: z.literal("object"), … }) but outputSchema as z.looseObject({ $schema: z.string().optional() }), with no type constraint. So a {"type":"string"} output schema is conforming, and the rule was asserting a requirement MCP does not make — at error severity, which would have exited --strict with 6 against a valid server. Now scoped to inputSchema, with the asymmetry cited in the doc comment so it isn't "tidied up" later.

#7 — the suggestion was wrong, not merely suboptimal. For a required type: ["null","boolean"] field, "use boolean and drop it from required" both stops accepting null and starts accepting omission. Accepting an explicit null and permitting absence are independent contracts. The suggestion is now always anyOf branches rendered from the real members — {"anyOf": [{"type": "null"}, {"type": "boolean"}]} — which is equivalent and which this lint already treats as portable. Optionality is mentioned only as an explicitly different contract, and a test asserts the old phrasing can't come back.

#5 — the test earned its keep immediately. Asking for live coverage of rawToolSchemas surfaced something no unit test could see: a conforming client validates a tool result against the advertised output schema, so putting an outputSchema override on a preset whose handler returns no structuredContent makes every call to that tool fail. The showcase config did exactly that to echo, so pressing Execute on the flagged tool would have errored — a fixture that breaks on its own obvious button. The bare true now rides get_temp (which does return structured content), the caveat is documented on ServerConfig.rawToolSchemas and in the README, and both halves are pinned by tests.

One judgement call flagged rather than silently taken, on #2: format is in the allowlist even though 2020-12's default vocabulary makes it an annotation rather than an assertion. Strictly {"format":"email"} does accept any value — but a schema declaring one is plainly an attempt to constrain, and reporting it as "accepts any value" would be noise rather than something an author can act on. Called out in the comment so the deviation is deliberate.

Screenshots and the TUI frame in the PR body are re-captured against the reshaped fixture. npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/cli/src/handlers/emit-result.ts:68

• The stable machine-readable code says the schema is invalid, but this feature deliberately reports schemas that are legal JSON Schema and merely unportable across clients. Automation consuming the envelope can therefore misclassify a portability failure as validation failure. Use a portability-specific code such as schema_unportable consistently.

 { code: "schema_invalid" },

core/json/schemaLint.ts:371

• {} accepts every JSON value, but the proposed “genuinely free-form” replacement accepts only objects. Applying this fix would reject strings, numbers, booleans, arrays, and null that the original schema allows. Use single-type anyOf branches to preserve an unconstrained JSON value.

 "Schema carries no validation keyword at all, so it accepts any value — the object-literal spelling of a bare `true`.",
'Give it an explicit `type`. For a genuinely free-form value use `{"type": "object", "additionalProperties": true}`.',

[cliffhall]

Review round 2 — both comments addressed (cac97c14)

Both were valid, and one of them deleted a rule rather than fixing it. Mirroring here because the inline threads go outdated on push.

non-object-root was unreachable — rule removed

The claim was strong enough to verify rather than accept on the reading, so I probed it: a live composable server advertising inputSchema: {type: "array", items: {type: "string"}}, connected over a real transport.

PROBE_TOOLS=[]

The tool never arrives. ToolSchema types inputSchema with type: literal("object"), so the entry fails safeParse inside salvageListItems and lands in malformed rather than valid. All three clients read the salvaged list, so no surface could ever hand such a tool to the lint.

That makes it worse than a missing rule: the README, AGENTS.md, and the PR body were all describing a check that could not run. Removed — gone from SchemaLintRule, lintSchema is now just the walk, and each doc site now explains why there is no such rule. Nothing is lost in coverage; the condition is already surfaced by the malformed-items path, which is where a dropped tool belongs. I chose that over detecting it in the raw/salvage path, which would mean a second raw tools/list in the style of refreshExcludedTools — a lot of machinery to re-report something the user is already told.

Two guards keep it honest: the live behaviour is pinned in raw-tool-schemas.test.ts with a comment saying that if it ever starts returning the tool, the rule is worth adding back and that test is the signal; and the unit suite asserts the module stays quiet on non-object roots rather than silently regrowing the check.

The false suggestion inverted the schema

properties: {a: false} forbids the property; deleting the entry permits it with any value under the default additionalProperties. The advice did the opposite of the schema it replaced. Now {"not": {}}, with the difference stated.

That needed one more change to avoid self-defeat: {"not": {}} would itself have tripped untyped-schema, since the walk descends into not and finds an empty schema. So the rule no longer fires when the parent keyword is not — under negation an always-accepting subschema means always-reject, so "accepts any value" would be exactly backwards. Scoped to that position only, not inherited by the subtree; both halves tested.

This is the same class of defect as the type-union suggestion in round 1, which I fixed only where it was pointed at instead of sweeping for the pattern. So this round I went through all of them: every suggestion now either preserves the contract or says plainly that it narrows deliberately. true in particular has no portable object-form equivalent — {} is the very shape this lint flags — so claiming otherwise would have been the wrong advice.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

README.md:308

• The “Try it” link points back to the current Unportable tool schemas heading; there is no Try it section in this README. This makes the promised caveat link misleading instead of navigating to a target. Refer directly to the caveat below.

returns structured content) rather than `echo` — see the caveat under
[Try it](#unportable-tool-schemas) below.

[cliffhall]

Fixed — and this one is squarely a self-inflicted contradiction: the module's own header argues at length that these schemas are valid JSON Schema and are merely refused by some clients, and then the machine-readable code said schema_invalid.

Renamed throughout, while it is still pre-ship and nothing depends on it:

• EXIT_CODES.SCHEMA_INVALID → SCHEMA_UNPORTABLE
• envelope code: "schema_invalid" → "schema_unportable"
• the message now reads N tool schema portability error(s) found (--strict) rather than N tool schema error(s), so the human line does not misclassify it either
• the exit-code map's doc comment and the CLI README's exit-code table both say why 6 is "unportable" and not "invalid"

The test now asserts the new code with a comment naming the reason, so a future rename back would have to argue with it rather than just pass.

[cliffhall]

Review round 3 — all 4 addressed (a3d864ed)

Three inline plus the suppressed one, all valid. Mirroring here since the inline threads go outdated on push.

schema_invalid misnamed the failure

The sharpest of the three, because it was a contradiction with the PR's own premise: the module argues at length that these schemas are valid JSON Schema and are merely refused by some clients, and then the machine-readable code said schema_invalid. An automated caller branching on it would have been told the wrong thing.

Renamed while nothing depends on it yet — EXIT_CODES.SCHEMA_UNPORTABLE, envelope schema_unportable, and the message now reads N tool schema portability error(s) found (--strict). The exit-code map's comment and the README's exit-code table both say why 6 is "unportable" and not "invalid", and the test asserts the new code with the reason attached.

The fixture was covered by nothing

Every case in the new integration suite built a ServerConfig by hand, so resolveConfig's rawToolSchemas forwarding was an uncovered line and unportable-schemas-http.json was loaded by no test at all — the shipped fixture could have silently lost its overrides with the whole suite still green.

Added a fixture-path case on the duplicate-tool-names.test.ts model: resolveConfig(loadConfig(configPath)), then boot from the resolved config and assert on the wire. It pins the exact per-tool verdict the README and the screenshots claim, not just "some findings exist":

{ tool: "get_temp", rules: ["boolean-schema"] },
{ tool: "echo", rules: ["type-union", "untyped-schema"] },
{ tool: "add", rules: ["remote-ref"] },

plus { errors: 1, warnings: 3 }. So docs-vs-fixture drift now fails a test.

The CLI README sample was stale

Left over from before round 2 reworded the suggestion, so it showed output the CLI no longer produces — and dropped exactly the contract warning that rewording existed to add. The tool name and counts were stale too, still describing the pre-round-2 fixture. Replaced with real output captured from a run against the showcase server rather than hand-written, so it matches verbatim, plus a short paragraph making the suggestion pattern explicit instead of leaving it to be inferred from one example.

Suppressed comment

Also fixed: the root README's [Try it](#unportable-tool-schemas) link pointed back at its own section. Now refers to the caveat directly.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

core/json/schemaLint.ts:179

• if, then, and else do not constrain a schema independently: if has no assertion effect without then/else, and then/else are ignored without if. Consequently schemas such as { "if": { "const": 1 } } and { "then": { "type": "string" } } still accept every value but evade untyped-schema. Remove these from the presence-only allowlist and count them only when if is paired with then or else.

 "if",
"then",
"else",

core/json/schemaLint.ts:338

• This branch treats every array-valued type as a legal union, including malformed values such as [], [3], duplicates, or unknown type names. The emitted issue then incorrectly says the schema is legal and may suggest an invalid or contract-changing replacement (the tests currently expect non-string members to be silently dropped). Restrict this portability rule to non-empty, unique arrays of recognized JSON Schema type names; malformed arrays should either be skipped here or reported by a separate validation rule.

 if (Array.isArray(type)) {
const members = type.filter((t): t is string => typeof t === "string");

clients/web/src/components/groups/ToolListItem/ToolListItem.tsx:98

• The two severities are visually distinguished only by color: both branches render the same warning icon, and the visible tooltip text does not say “error” or “warning.” Color-blind users therefore cannot tell the severity from this row. Use distinct icon shapes/text (for example ! versus ?) or another always-visible non-color cue, while retaining the accessible label.

 c={
hasError
? "var(--inspector-danger-text)"
: "var(--inspector-warning-text)"
}

clients/cli/README.md:204

• The documented sample count is stale: the showcase and the tested report contain findings across three tools (get_temp, echo, and add), not two. Update this line to match the actual CLI output.

error, 3 warnings across 2 tools. Re-run with --strict for details.` — and

[cliffhall]

Review round 4 — all 6 addressed (2cb23a8b)

Two inline plus four suppressed, all valid. Mirroring here since the inline threads go outdated on push.

add was not actually runnable in the TUI

The one I'd have been most embarrassed to ship. I verified "every showcase tool is runnable" over the wire and against the web form, and never opened schemaToForm — which dispatches on type, so the $ref-only property fell through to a string field that add's numeric handler rejects. The README and the PR both made that claim, so it was false, not rough.

The override now keeps a local type alongside the remote reference, and restores the preset's required (which I had silently dropped on echo too). The remote-ref finding is unaffected — that rule reads $ref independently of type.

Two tests hold the claim up rather than my word: schemaToForm.test.ts asserts the property becomes a numeric field rather than the string fallback, and raw-tool-schemas.test.ts now calls every tool in the fixture with the arguments its advertised schema renders — including the properties the override added — since that is what a user clicking through the form sends. Writing the first also turned up that the TUI's numeric field kind is float, not number, so the assertion pins the real contract instead of an assumed one.

The new exit code was missing from codeForExit

Exactly the defect the round-3 rename was meant to prevent, one layer down: CliExitCodeError lets a caller omit the envelope, so an envelope-less throw reported the generic error. Added the case and covered it through the envelope-less construction that exposed it.

Suppressed comments — all four fixed

• type-union fired on malformed arrays. Its message asserts the construct is legal JSON Schema, which is false for [] (matches nothing), [3], ["bananas"], or a duplicated member — and the anyOf it suggested for those would be invalid too. Now restricted to non-empty, unique arrays of recognized type names. Same principle the walk already applies to a node that is neither object nor boolean: malformed is the SDK parser's business, not this module's.
• if/then/else were in the presence-only allowlist. They constrain only as a pair — if asserts nothing without a then/else, and either is ignored without an if — so {"if": {"const": 1}} accepts every value and evaded untyped-schema. The pairing is now checked explicitly. This is the same allowlist reasoning from round 1, applied to the one keyword group where presence alone is not enough.
• Severity was carried by colour alone in the web sidebar: both branches rendered the same icon and the tooltip said "finding". Now a circle for an error and a triangle for a warning — a real shape difference — with the severity named in the tooltip as well as the accessible label. The TUI already did this with ! vs ?. Screenshot in the PR body re-captured.
• Stale "across 2 tools" in the CLI README's hint sample: fixed.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

core/json/schemaLint.ts:412

• A non-fragment $ref does not necessarily point outside the document. For example, when the root (or an embedded resource) has $id: "https://example.com/root", $ref: "https://example.com/root#/$defs/x" resolves to the already bundled resource and requires no remote fetch. This check flags that valid local reference and gives incorrect remediation. Track in-document $id resources and resolve the reference before classifying it as remote.

 const ref = node.$ref;
if (typeof ref === "string" && ref !== "" && !ref.startsWith("#")) {

clients/cli/src/cli.ts:1014

• This validation runs after the servers/list and servers/show returns (lines 901–921) and the other short-circuit modes, so invocations such as --strict --method servers/list or --strict --list-stored-auth succeed while silently ignoring a flag documented as tools/list-only. Move the strict/method validation before every short-circuit return (and before stored-auth work), and cover one catalog or short-circuit invocation.

 if (options.strict && options.method !== "tools/list") {
throw new Error("--strict requires --method tools/list.");
}

[cliffhall]

Review round 5 — all 3 addressed (68c4abf3)

One inline plus two suppressed. Mirroring here since the inline thread goes outdated on push.

The tooltip miscounted mixed severities — and I introduced it last round

hasError was applied to the total, so a tool with one error and three warnings announced as "4 schema portability errors". That is worse than the vague "N findings" wording it replaced in round 4, because it is now confidently wrong instead of merely unspecific — and I added it while fixing a different accessibility problem on the same line.

Both the tooltip and the accessible label now carry the breakdown — Schema portability: 1 error, 3 warnings — with empty categories omitted. The formatting moved into a shared summarizeToolFindings in core/json/schemaLint.ts rather than staying inline: this is the fourth place that has had to phrase a severity count, and inlining it is exactly how the four drift apart. ToolListItem.test.tsx gained a mixed-severity case asserting the label is not "2 errors".

The icon still keys off hasError, which is correct — that is the highest severity and one glyph can only carry one. It was only the count that must not inherit it.

remote-ref fired on a local reference

A non-fragment $ref is not necessarily remote: $id establishes a base URI, so with a root $id of https://example.com/root, the ref https://example.com/root#/$defs/x resolves to this document and needs no fetch. The finding and its "inline the referenced schema" remediation would both have been wrong on a valid schema.

Classifying refs correctly there means full RFC 3986 base-URI resolution against possibly-nested embedded resources, which this module does not do. So it now declines to classify when any $id is declared anywhere in the document, rather than guessing — the same trade already made for malformed type arrays and for the removed root rule. A missed finding is the acceptable direction; a wrong one with wrong advice is not. $id is vanishingly rare in a tool schema, so this costs almost nothing, and the comment records what the refinement would be if it ever matters.

--strict was accepted-and-ignored on the short-circuit paths

servers/list, servers/show, --list-stored-auth and --print-handoff all return from parseArgs before any connect, so validations placed further down never ran for them: --strict --method servers/list succeeded while silently ignoring a flag documented as tools/list-only. Both --strict checks now run ahead of every short-circuit, covered by a table-driven case through the real runCli.

That is the same "accepted but inert" failure the --app-info pairing rejection exists to prevent — I fixed the instance in round 3 and did not check whether the placement of the check had the same hole.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

clients/cli/README.md:173

• This block is presented as the command's output, but formatSchemaLintReport emits one block for every finding before the summary. The documented fixture has three warning findings, so the summary can never immediately follow this error block as shown. Include the warning blocks or mark the middle as omitted so the example does not claim an output shape the CLI cannot produce.

Error: tool "get_temp"
Path: outputSchema.properties.data
Issue: Bare `true` used where a schema object is expected.
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 3 tools.

[cliffhall]

Review round 6 — all 3 addressed (488106f2)

Two inline plus one suppressed. Mirroring here since the inline threads go outdated on push.

The $id guard could be switched off by instance data

Third round running where the previous round's fix is the next round's defect — and this one inverted its own purpose. The $id scan I added in round 5 to prevent a false positive descended through every object value, including instance-valued keywords. examples: [{"$id": "payload-field"}] is data, not an embedded resource, but reading an $id there suppressed remote-ref for the whole document: a targeted false-positive fix became a blanket false negative.

Rather than copying the keyword tables into the pre-scan, I extracted the traversal both passes share — forEachSubschema, driven by the same SUBSCHEMA_MAP_KEYWORDS / SUBSCHEMA_ARRAY_KEYWORDS / SUBSCHEMA_KEYWORDS (tuple-items case included). walk recurses through it, declaresAnyId scans through it, so they cannot disagree about what a schema position is. Making the scan matchwalk by hand would have fixed today's bug and left the same shape in place for the next keyword added to one list and not the other.

The report could be truncated or lost

writeSchemaLintReport used a bare process.stderr.write. Both CLI exit paths call process.exit() as soon as the work returns, and on a pipe or a file — not a TTY — the write is asynchronous, so whatever is still buffered is discarded. Worst on the multi-block --strict report, and on precisely the redirected-output runs a CI caller uses.

The repo already had the answer and I walked past it: awaitableLog wraps stdout for exactly this reason. Added awaitableError beside it, made the function async, awaited it in emitResult before the throw. The test's fake returns false and defers its callback to a later tick — a fake that called back synchronously would have passed with or without the await. Writing it also turned up that the file's two existing stderr stubs never invoked their callbacks at all, so they would have hung the moment the function started awaiting.

The README sample showed an impossible shape

The block printed one error finding immediately followed by the summary, but the formatter emits a block for every finding — with three warnings in the fixture, that output cannot occur. Replaced with the complete real report, captured from a run rather than written by hand.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

[cliffhall]

Review round 7 — all 3 addressed (31bdd532)

Three inline comments, no suppressed ones. Mirroring here since the inline threads go outdated on push.

contentSchema was missing from the traversal

A standard 2019-09/2020-12 schema-valued keyword, absent from every table — so a bare true or a remote $ref under it was silently walked past by all three clients. Added, with a walk-position case.

I also admitted the three content keywords (contentMediaType, contentEncoding, contentSchema) to CONSTRAINING_KEYWORDS, which is the other half of getting this right. They are annotation-only in 2020-12's default vocabulary, so strictly a schema carrying one accepts any value — but that is the same call already made and documented for format, and calling such a schema "constrains nothing" would be noise. The comment now names that category instead of singling format out, so the next annotation-shaped keyword lands in an existing rationale rather than being decided ad hoc.

The envelope write raced process.exit()

This is the sharpest one: round 6 made the --strictreport await its write, and stopped one line short of the actual exit. The ErrorEnvelope emitted for the very same throw — the machine-readable line a CI caller parses — was still fire-and-forget. I fixed the symptom I had been shown and not the exit path it flows into.

handleError is now async and awaits before exiting. The launcher's call site awaits too, and I replaced the comment there that justified not awaiting: it argued the envelope is "a few hundred bytes, well inside the pipe buffer". That reasoning is wrong — fitting in the buffer says nothing about whether the write has been performed when the process goes away — and leaving it would have invited someone to drop the await again. The bin's .catch(handleError) needed no change; formatErrorOutput stays sync, so the in-process test runner is unaffected.

The new test's spy returns false and defers its callback, asserting the flush happened before process.exit. A synchronous spy passes with or without the await, which is exactly how this class stays hidden — and writing it surfaced that the two existing spies never invoked their callbacks at all.

"Complete output" wasn't

My wording from last round, added while fixing a different honesty problem in the same block. The error envelope follows the report on every non-zero exit, so the block was not the complete stderr. Now scoped to "the complete report", with the envelope named, linked, and its schema_unportable code shown.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (1)

core/json/schemaLint.ts:425

• The claim that this replacement “narrows” the contract is false in non-monotonic positions. For example, {"not": true} currently rejects every value, but replacing the child with the suggested object schema makes the parent allow every non-object; an if: true replacement can similarly let values bypass then. Use parent-aware advice for not (where {} is an exact object-form replacement and is already exempted below), and describe the generic replacement as changing—not necessarily narrowing—the enclosing contract.

 '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.'

[cliffhall]

Review round 8 — no new inline comments; the one suppressed finding is fixed (182c48f2)

Copilot generated no new comments this round. It did carry one suppressed finding, and it is correct and worth fixing rather than waving through.

"Narrows" was false in non-monotonic positions

The boolean-schema suggestion for a bare true claimed the replacement "narrows the schema deliberately". That holds in an ordinary property position and inverts under a negating parent: {"not": true} rejects every value, so swapping in {"type": "object", …} leaves the parent rejecting only objects and every non-object now passes. The enclosing contract is widened. Under if it neither narrows nor widens — it redirects which branch applies.

Two changes:

• Parent-aware advice under not.{} and true are the same schema, so {} is always the exact rewrite; it is normally bad advice only because {} is the shape untyped-schema flags. Under not that exemption already exists (from round 5), so {} is both exact and safe to recommend there — and the message says why declaring a concrete type would be wrong in that position.
• "Deliberate change of contract", not "narrows", everywhere else — in both the boolean-schema and untyped-schema suggestions. A suggestion has no business claiming an effect it cannot guarantee across positions.

Tests: a case pinning the not-specific advice, and one asserting that no suggestion for a bare true in any of the three positions contains the word "narrow". The CLI README's sample report was re-captured from a real run so the quoted text matches.

---

This is the eighth round, and the seven before it found a defect every time — five of them in the previous round's fix. Worth stating plainly what that pattern was, since it is the same one throughout: I kept validating a change against the problem it was asked to solve, not against what the change itself then asserted. The $id guard checked that it stopped a false positive but not that data could trigger it; the awaited report write checked the report but not the envelope on the same throw; this suggestion checked that it was honest about not being an equivalent rewrite, but not that "narrows" was true in every position it can print from.

npm run ci green.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.