From 81f2115e8b9c350fb628c7dfe451f3708f7a59ef Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 12:59:52 -0400 Subject: [PATCH] fix: decline a union branch requiring a name it never declares (#2224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isOfferable` in `core/json/rootUnion.ts` checked that a merged branch carries at least one property and that every property value is readable, but never that each name in `required` resolves to a property a form can render. A branch like `{ properties: { kind }, required: ["kind", "payload"] }` was therefore offered in the picker, rendered no control for `payload`, and had the submit-time check report it missing permanently — the exact dead end the function's own comment already reasons through for a `false`-schema required field, one case short. Judge the merge, so a branch requiring only names the ROOT declares — `anyOf: [{ required: ["email"] }, …]` — stays offerable as before. `hasOwn` rather than `in`, since an argument legally named `constructor` would otherwise resolve through the prototype and read as declared. Declining every branch of a union leaves the web form with neither a picker nor a field, which is a different dead end. `SchemaForm` now seeds the raw-JSON editor (#2151) in that one case — a root union that resolved to no branches and no properties — so the arguments stay expressible. Seeded rather than forced, so the switch still works, and re-seeded during render (never in an effect) because the form is reused across tools rather than remounted. A tool that genuinely takes no arguments is untouched. The TUI has no such editor; there the union is simply not offered, which is already better than a section that can never be submitted. Adds a `record_shipment_by` preset and wires it into the `root-union-schemas-http.json` showcase so the shape is reproducible by hand, with docs/test-servers.md covering it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PxYT4eKdnx1yRw7x4bW7Ts Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 52 ++++++++++++ .../groups/SchemaForm/SchemaForm.test.tsx | 80 +++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 21 ++++- clients/web/src/test/core/rootUnion.test.ts | 78 ++++++++++++++++++ core/json/rootUnion.ts | 22 ++++- docs/test-servers.md | 7 +- .../configs/root-union-schemas-http.json | 26 +++++- test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 29 +++++++ 9 files changed, 312 insertions(+), 6 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index b245132e..2ea524db 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -656,6 +656,58 @@ describe("schemaToForm", () => { expect(names).toContain("__b_0____b0__x"); }); + // #2224: `required` lists names, not declarations, so a member may require + // one it never declares. `buildFields` enumerates `properties` alone, so no + // control was rendered for it while `missingRequiredFields` reported it + // missing at every submit — a section the user could never complete. + it("declines a member requiring a name it never declares", () => { + const schema = { + type: "object", + anyOf: [ + { + type: "object", + properties: { kind: { type: "string", const: "a" } }, + required: ["kind", "payload"], + }, + { + type: "object", + properties: { kind: { type: "string", const: "b" } }, + required: ["kind"], + }, + ], + }; + const form = schemaToForm(schema, "undeclared_required"); + // No variant select and no per-branch section: the union is declined, so + // the root's own (here empty) properties are what render. + expect(form.sections).toEqual([{ title: "Parameters", fields: [] }]); + // And nothing is reported missing, so the call is no longer blocked on a + // field that has nowhere to be typed. + expect( + missingRequiredFields(schema, decodeFormValues(schema, {})), + ).toEqual([]); + }); + + it("keeps offering a member requiring a name the root declares", () => { + // The regression guard for the check above: the merge is what is judged, + // and `anyOf: [{ required: ["email"] }, …]` is an ordinary union. + const form = schemaToForm( + { + type: "object", + properties: { + email: { type: "string" }, + phone: { type: "string" }, + }, + anyOf: [ + { type: "object", required: ["email"] }, + { type: "object", required: ["phone"] }, + ], + }, + "inherited_required", + ); + expect(form.sections).toHaveLength(3); + expect(form.sections[0]!.fields[0]).toMatchObject({ name: "__variant" }); + }); + describe("decodeFormValues", () => { it("submits the chosen branch's fields under their real names", () => { expect( diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 518c5f4c..e8b23b6d 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2284,6 +2284,86 @@ describe("SchemaForm raw JSON (#2151)", () => { await enableRawJson(user); expect(screen.getAllByLabelText("Edit as JSON")).toHaveLength(1); }); + + // #2224: a root union whose alternatives are all declined resolves to a base + // with nothing on it, so the form renders no picker and no fields. The switch + // is the only way to make the call — open it rather than leaving the user in + // front of a blank form to work that out. + describe("fallback for an unrenderable root union (#2224)", () => { + // Every member requires a name it never declares, so none is offerable. + const deadEnd: InspectorFormSchema = { + type: "object", + anyOf: [ + { + type: "object", + properties: { kind: { type: "string", const: "a" } }, + required: ["kind", "payload"], + }, + { + type: "object", + properties: { kind: { type: "string", const: "b" } }, + required: ["kind", "payload"], + }, + ], + }; + + function switchElement(): HTMLInputElement { + return screen.getByLabelText("Edit as JSON") as HTMLInputElement; + } + + it("opens the editor when the form would otherwise be empty", () => { + renderWithMantine(); + expect(switchElement().checked).toBe(true); + expect(getAceTextByLabel(/Arguments JSON/)).toBe("{}"); + }); + + it("leaves the editor closed for a tool that takes no arguments", () => { + // Nothing to render here either, but nothing is missing: seeding a JSON + // editor for `{}` would be noise on every no-argument tool. + renderWithMantine( + , + ); + expect(switchElement().checked).toBe(false); + }); + + it("still lets the user switch back to the fields", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(switchElement()); + expect(switchElement().checked).toBe(false); + }); + + it("opens the editor for a schema that arrives after the first render", () => { + // The form is reused across tools rather than remounted, so a `useState` + // initializer alone would only ever see the tool it mounted on. + const { rerender } = renderWithMantine( + , + ); + expect(switchElement().checked).toBe(false); + + rerender(); + expect(switchElement().checked).toBe(true); + }); + + it("does not close an editor the user opened when the schema changes", async () => { + // One-way: the fallback opens the editor, and nothing closes it but the + // user. + const user = userEvent.setup(); + const { rerender } = renderWithMantine( + , + ); + await enableRawJson(user); + + rerender( + , + ); + expect(switchElement().checked).toBe(true); + }); + }); }); // A plain `` swallows Enter, so a string argument could not be given a diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 60baca60..bfaade84 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -931,7 +931,26 @@ export function SchemaForm({ // rather than reported under a reserved name: field names come straight out // of a server's schema, so any sentinel this form invented could collide with // a real argument and clear a block the user cannot see. - const [rawJsonMode, setRawJsonMode] = useState(false); + // + // A root union whose alternatives are all declined resolves to a base with + // nothing on it, so the form renders neither a picker nor a field — the user + // is left staring at a switch they have to know to reach for in order to make + // the call at all (#2224). Open the editor for them in that one case: the + // arguments are still expressible, just not as fields. Seeded rather than + // forced, so the switch keeps working; re-seeded during render (never in an + // effect) because this form is reused across tools rather than remounted, and + // a `useState` initializer would only ever see the first one. + const rawJsonFallback = + allowRawJson && + branches.length === 0 && + (schema.oneOf !== undefined || schema.anyOf !== undefined) && + Object.keys(properties).length === 0; + const [rawJsonMode, setRawJsonMode] = useState(rawJsonFallback); + useValueChange(rawJsonFallback, (fallback) => { + // One-way: a schema that stops needing the fallback does not close an + // editor the user may have opened on purpose. + if (fallback) setRawJsonMode(true); + }); const [rawJsonInvalid, setRawJsonInvalid] = useState(false); // Stable so `RawArgumentsField`'s reporting effect subscribes once rather diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index 830b2b2e..e25ff630 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -809,6 +809,84 @@ describe("resolveRootUnion", () => { expect(branches).toHaveLength(2); }); + it("declines a member requiring a name nothing declares", () => { + // `required` is a list of names, not of declarations. Both form builders + // enumerate `properties` alone, so `payload` gets no control while the + // submit gate reports it missing forever — the picker offers an option + // that can never be completed (#2224). + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { kind: { const: "a" } }, + required: ["kind", "payload"], + }, + SMS, + ], + }); + expect(branches).toEqual([]); + }); + + it("declines a member requiring a name only ANOTHER member declares", () => { + // Branches are alternatives, not a conjunction: a name the sibling + // declares is not one this branch can render, so the dead end is the + // same one. + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { kind: { const: "a" } }, + required: ["kind", "phone"], + }, + SMS, + ], + }); + expect(branches).toEqual([]); + }); + + it("offers a member requiring a name it inherits from the root", () => { + // The merge is what is judged, so a member requiring a name the ROOT + // declares stays offerable — the `anyOf: [{ required: ["email"] }, …]` + // shape above must not regress. + const { branches } = resolveRootUnion({ + type: "object", + properties: { token: { type: "string" } }, + anyOf: [ + { + type: "object", + properties: { a: { type: "string" } }, + required: ["token"], + }, + { + type: "object", + properties: { b: { type: "string" } }, + required: ["token"], + }, + ], + }); + expect(branches).toHaveLength(2); + }); + + it("does not read an inherited name as a declared property", () => { + // An argument legally named `constructor` resolves through the prototype + // on a plain object, so a membership test that is not `hasOwn` would call + // this branch offerable and render nothing for it. + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { kind: { const: "a" } }, + required: ["constructor"], + }, + SMS, + ], + }); + expect(branches).toEqual([]); + }); + it("declines a member carrying a `false` property schema", () => { // `false` admits no value at all, so the field can never be filled — and // a required one makes the branch unsatisfiable. diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 7921824c..bf3a1e26 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -162,7 +162,7 @@ function propertiesOf(schema: RootUnionSchema): Record { /** * Whether a branch is one a form can offer as an alternative. * - * Three ways it is not, all of which would put an option in the picker that + * Four ways it is not, all of which would put an option in the picker that * cannot be filled in: * * - **It carries no fields.** A `{ type: "null" }` member — the nullable @@ -174,6 +174,12 @@ function propertiesOf(schema: RootUnionSchema): Record { * - **Its `type` rules objects out.** Tool arguments are a JSON object, so a * `{ type: "string", properties: {…} }` member can never match — rendering it * as a fillable form would offer a call that cannot be valid. + * - **It requires a name it never declares.** `required` is a list of names, + * not of declarations, so `{ properties: { kind: … }, required: ["kind", + * "payload"] }` is legal and says nothing about what `payload` accepts. Both + * form builders enumerate `properties` alone, so no control is rendered for + * it and the submit-time check reports it missing *permanently* — the same + * dead end a `false`-schema required field produces (#2224). */ function isOfferable( branch: RootUnionSchema, @@ -191,7 +197,19 @@ function isOfferable( // a perfectly ordinary way to say "one of these two" — and judging it on its // own properties would decline it, leaving the gate checking the base alone // and accepting `{}`, which the schema rejects. - const properties = Object.values(propertiesOf(merged)); + const mergedProperties = propertiesOf(merged); + // Judged on the MERGE for the same reason the fields are: a member may + // require a name the root declares, which is exactly the + // `anyOf: [{ required: ["email"] }, …]` shape above. `hasOwn` rather than + // `in`, since an argument legally named `constructor` or `toString` would + // otherwise resolve to the inherited one and read as declared. + if ( + requiredOf(merged).some((name) => !Object.hasOwn(mergedProperties, name)) + ) { + return false; + } + + const properties = Object.values(mergedProperties); return ( properties.length > 0 && // Every value has to be something a renderer can read AND something a diff --git a/docs/test-servers.md b/docs/test-servers.md index 4e214f34..1330e72a 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -44,7 +44,7 @@ as a missing capability rather than an error. | `structured-output-http.json` **(legacy era)** | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` **(legacy era)** | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | | `nullable-fields-http.json` **(legacy era)** | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | -| `root-union-schemas-http.json` **(legacy era)** | Tool schemas whose arguments are a root `anyOf` / `oneOf` | [#2123](https://github.com/modelcontextprotocol/inspector/issues/2123) | +| `root-union-schemas-http.json` **(legacy era)** | Tool schemas whose arguments are a root `anyOf` / `oneOf`, including one no branch of which can be offered | [#2123](https://github.com/modelcontextprotocol/inspector/issues/2123), [#2224](https://github.com/modelcontextprotocol/inspector/issues/2224) | | `unportable-schemas-http.json` **(legacy era)** | Tool schemas a real client rejects, flagged in all three clients | [#1005](https://github.com/modelcontextprotocol/inspector/issues/1005) | | `rfc6570-templates-http.json` **(legacy era)** | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` **(legacy era)** | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | @@ -271,7 +271,7 @@ The **TUI** had the same gap and is worth checking against the same server (`--t ## Root-level unions -`root-union-schemas-http.json` serves two tools whose arguments are declared as a **composition at the root** of `inputSchema` rather than as a flat `properties` map — `echo` with an `anyOf` beside its own `message` property, and `get_weather` with an OpenAPI-style `discriminator` over a `oneOf`. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. +`root-union-schemas-http.json` serves three tools whose arguments are declared as a **composition at the root** of `inputSchema` rather than as a flat `properties` map — `echo` with an `anyOf` beside its own `message` property, `get_weather` with an OpenAPI-style `discriminator` over a `oneOf`, and `record_shipment_by` with a `oneOf` neither of whose branches can be offered. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. The 2026-07-28 revision makes this shape explicitly legal: `type: "object"` is required at the root, and beyond that "any JSON Schema 2020-12 keyword may appear alongside `type`, including composition keywords (`oneOf`, `anyOf`, `allOf`, `not`)". @@ -292,6 +292,7 @@ All three read one helper, [`core/json/rootUnion.ts`](../core/json/rootUnion.ts) What it declines to flatten is as deliberate as what it flattens, and every case falls back to whatever the schema's own `properties` describe rather than claiming something untrue: - **A union whose members are not all field-carrying object schemas** — including one whose member `type` rules objects out, since tool arguments are a JSON object and such a member can never match. A picker whose options render nothing is no better than no picker. +- **A branch requiring a name nothing declares.** `required` lists names, not declarations, so `{ "properties": { "by": … }, "required": ["by", "address"] }` is legal and says nothing about what `address` accepts. Every form builder enumerates `properties` alone, so no control is rendered for it while the submit-time check reports it missing *permanently* — an option the picker offers and the user can never complete ([#2224](https://github.com/modelcontextprotocol/inspector/issues/2224)). The name only has to be declared *somewhere the merge reaches*: a branch requiring one the **root** declares — `anyOf: [{ "required": ["email"] }, …]`, an ordinary way to say "one of these two" — is offered as before. - **A branch that restates a constraint the root already states.** The two are conjunctive, so root `minimum: 10` under branch `minimum: 0` is still 10, disjoint `enum`s leave nothing satisfiable, and `type: "string"` under `type: "number"` describes a value that cannot exist — rendering either side would accept what the schema rejects. A property both declare *compatibly* is merged rather than replaced, so a root's `minimum` survives a branch's `maximum`, and a disagreement about `title`/`description` is not a conflict at all. - **A composition member stating anything the merge cannot apply.** Only `type`, `properties` and `required` are folded in, so a member carrying a nested `allOf`/`anyOf`, a `not`, an `additionalProperties`, or a `$ref` would have that constraint erased along with the keyword — turning an unsatisfiable schema (`allOf: [false, …]` admits nothing) into a fillable form. `allOf` members are checked against the accumulated merge rather than the root alone, so two of them contradicting each other is caught even when neither contradicts the root. - **A `oneOf` whose alternatives are not mutually exclusive.** `oneOf` demands that *exactly one* alternative match, which flattening cannot preserve — the branches are offered as if any would do. It is only safe with a discriminator: a property every branch pins to a `const` of its own **and requires**, since an optional one leaves `{}` matching every branch. An undiscriminated `oneOf` is declined; `anyOf` makes no such claim and is offered either way. @@ -301,6 +302,8 @@ What it declines to flatten is as deliberate as what it flattens, and every case Declining changes what *renders*, never whether the tool is treated as taking arguments: a declined union still has fields, so an App tool carrying one still asks for them rather than auto-invoking with `{}`. +`record_shipment_by` is the case where declining leaves nothing on screen: its fields live entirely on branches that are all declined, so the web form has neither a picker nor a field to show. It opens the **Edit as JSON** editor instead of rendering a blank form the user would have to work out for themselves ([#2224](https://github.com/modelcontextprotocol/inspector/issues/2224)) — the arguments are still expressible, just not as fields. The switch is only *seeded*, so turning it back off works normally, and a tool that genuinely takes no arguments is left alone. The TUI has no such editor, so there the tool renders an empty Parameters section — no longer a form with a branch section that could never be submitted. + ## Unportable tool schemas `unportable-schemas-http.json` serves four tools, three of whose advertised diff --git a/test-servers/configs/root-union-schemas-http.json b/test-servers/configs/root-union-schemas-http.json index 33896b74..15bf728c 100644 --- a/test-servers/configs/root-union-schemas-http.json +++ b/test-servers/configs/root-union-schemas-http.json @@ -3,7 +3,11 @@ "name": "root-union-schemas-showcase", "version": "1.0.0" }, - "tools": [{ "preset": "echo" }, { "preset": "get_weather" }], + "tools": [ + { "preset": "echo" }, + { "preset": "get_weather" }, + { "preset": "record_shipment_by" } + ], "rawToolSchemas": { "echo": { "inputSchema": { @@ -32,6 +36,26 @@ ] } }, + "record_shipment_by": { + "inputSchema": { + "type": "object", + "discriminator": { "propertyName": "by" }, + "oneOf": [ + { + "type": "object", + "title": "By address", + "properties": { "by": { "type": "string", "const": "address" } }, + "required": ["by", "address"] + }, + { + "type": "object", + "title": "By tracking number", + "properties": { "by": { "type": "string", "const": "tracking" } }, + "required": ["by", "tracking"] + } + ] + } + }, "get_weather": { "inputSchema": { "type": "object", diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index 26fa8b9a..fe0f92f6 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -16,6 +16,7 @@ import { createGetEnvTool, createAddTool, createGetSumTool, + createDeadEndUnionTool, createGetWeatherTool, createInvalidHeaderTool, createSpecErrorTriggerTool, @@ -111,6 +112,8 @@ function resolveToolPreset( return createGetSumTool(); case "get_weather": return createGetWeatherTool(); + case "record_shipment_by": + return createDeadEndUnionTool(); case "invalid_header_tool": return createInvalidHeaderTool(); case "trigger_header_mismatch": diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 8bef0410..9db734ab 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -272,6 +272,35 @@ export function createGetWeatherTool(): ToolDefinition { }; } +/** + * Create a tool whose advertised arguments are a root `oneOf` **no branch of + * which can be offered**, for #2224. Each branch requires a name it never + * declares, so a form builder that enumerates `properties` alone renders no + * control for it while the submit gate reports it missing forever. + * + * The Zod `inputSchema` here is a placeholder: the shape that matters is the + * raw JSON Schema `root-union-schemas-http.json` substitutes through + * `rawToolSchemas`, since Zod cannot emit a root composition. The handler is + * what a real server would do with the arguments — echo them back, so what the + * form actually sent is visible in the result. + */ +export function createDeadEndUnionTool(): ToolDefinition { + return { + name: "record_shipment_by", + description: + "Record a shipment, by address or by tracking number. Its advertised schema is a root oneOf whose branches each require a name they never declare, so no branch is renderable.", + inputSchema: { + by: z + .string() + .optional() + .describe("Which alternative the call is making"), + }, + handler: async (params: Record) => { + return toToolResult(`Recorded shipment: ${JSON.stringify(params)}`); + }, + }; +} + /** * Create a tool whose SEP-2243 `x-mcp-header` annotation is INVALID: the header * name `"Bad Header"` contains a space, so it is not a valid RFC 9110 token.