diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index e3bc2b3c..287da67b 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -1022,6 +1022,26 @@ describe("SchemaForm nullable unions", () => { expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); }); + // Mantine's select is string-valued, so a numeric const would submit "1" for + // 1 — the same wrong-type-on-the-wire problem that keeps a numeric enum off + // the select path. + it("falls back to the JSON input for non-string anyOf consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: 1 }, { const: 2 }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + it("falls back to the JSON input when two anyOf branches share a const", () => { const schema: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 9c203901..9cba4bd9 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -60,10 +60,14 @@ function toEnumData( * branches that are expected to be constants. * * Returns `null` when the branches are not usable as options, which sends the - * field to the JSON fallback instead. Three ways that happens, all of which - * Mantine reacts to badly: + * field to the JSON fallback instead. Four ways that happens: * - * - **No `const`.** An `anyOf` of *object* schemas — what + * - **A non-string `const`.** Mantine's select is string-valued, so + * `anyOf: [{ const: 1 }, { const: 2 }]` would submit `["1"]` where the + * schema says `[1]` — the same wrong-type-on-the-wire problem that keeps a + * numeric `enum` off the select path. An inspector must not misreport what + * it sends, so this stays on the JSON editor, where the value keeps its type. + * - **No `const` at all.** An `anyOf` of *object* schemas — what * `z.array(z.union([z.object(…), z.object(…)]))` compiles to — has no * top-level `const` on any branch, so every option would be the empty * string. Mantine **throws** on duplicate option values, which greys out the @@ -80,10 +84,10 @@ function toConstOptions( const options: { value: string; label: string }[] = []; const seen = new Set(); for (const branch of branches) { - if (branch.const === undefined || branch.const === null) { + if (typeof branch.const !== "string") { return null; } - const value = String(branch.const); + const value = branch.const; if (value === "" || seen.has(value)) { return null; } diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index 915e7e8e..485febf1 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from "vitest"; -import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { + admitsNull, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; // The two JSON Schema encodings of "this type, or null". Both form builders — // the web `SchemaForm` and the TUI's `schemaToForm` — dispatch on a single @@ -281,3 +284,48 @@ describe("normalizeNullableUnion", () => { expect(normalizeNullableUnion(schema)).toBe(schema); }); }); + +// `admitsNull` answers a *validity* question and is deliberately decoupled from +// the collapse, which answers a narrower *rendering* one. A schema can admit +// null while being unrenderable as a single widget, and a form that conflated +// the two would reject a value its own schema accepts. +describe("admitsNull", () => { + it("recognizes every encoding the collapse handles", () => { + expect(admitsNull({ anyOf: [{ type: "string" }, { type: "null" }] })).toBe( + true, + ); + expect(admitsNull({ type: ["string", "null"] })).toBe(true); + expect(admitsNull({ type: "null" })).toBe(true); + expect(admitsNull({ nullable: true })).toBe(true); + }); + + it("recognizes a null branch the collapse declines to flatten", () => { + // Three members, so `normalizeNullableUnion` leaves this alone and it + // renders through the JSON fallback — where a user can still enter `null`. + const wide = { + anyOf: [{ type: "string" }, { type: "number" }, { type: "null" }], + }; + expect(normalizeNullableUnion(wide)).toBe(wide); + expect(admitsNull(wide)).toBe(true); + }); + + it("recognizes a null branch in a oneOf, and a nested type array", () => { + expect(admitsNull({ oneOf: [{ type: "string" }, { type: "null" }] })).toBe( + true, + ); + expect(admitsNull({ anyOf: [{ type: ["string", "null"] }] })).toBe(true); + }); + + it("is false for a schema that does not permit null", () => { + expect(admitsNull({ type: "string" })).toBe(false); + expect(admitsNull({ type: ["string", "number"] })).toBe(false); + expect( + admitsNull({ anyOf: [{ type: "string" }, { type: "number" }] }), + ).toBe(false); + expect(admitsNull({})).toBe(false); + }); + + it("ignores non-object anyOf members instead of throwing", () => { + expect(admitsNull({ anyOf: ["null", ["null"], 7] })).toBe(false); + }); +}); diff --git a/clients/web/src/test/integration/mcp/nullable-fields.test.ts b/clients/web/src/test/integration/mcp/nullable-fields.test.ts new file mode 100644 index 00000000..0a54e142 --- /dev/null +++ b/clients/web/src/test/integration/mcp/nullable-fields.test.ts @@ -0,0 +1,122 @@ +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 { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { toFormSchema } from "../../../utils/jsonUtils"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + createNullableFieldsTool, +} from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of the `record_shipment` preset behind + * `test-servers/configs/nullable-fields-http.json` — the documented manual + * reproduction for #1928. + * + * The unit tests on both form builders construct the `anyOf` shape by hand, + * which verifies the *renderers* but assumes the premise: that Zod's + * `.nullish()` actually emits `anyOf: [, { type: "null" }]` with the + * `enum` on the branch. That premise is the whole reason the fix exists, and + * nothing else pins it — a Zod change in how it compiles a nullish enum, or a + * typo in the preset registry, would leave every unit test green while the + * showcase server quietly stopped reproducing the bug. + * + * So this asserts the wire shape end to end, then runs it through the same + * collapse the forms use. + */ +describe("nullable argument schemas over the wire (#1928)", () => { + 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 connectToPreset(): Promise { + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("nullable-fields-test", "1.0.0"), + tools: [createNullableFieldsTool()], + }); + await started.start(); + server = started; + + const connected = new InspectorClient( + { type: "streamable-http", url: started.url }, + { environment: { transport: createTransportNode } }, + ); + await connected.connect(); + client = connected; + return connected; + } + + it("emits an anyOf-with-null for every nullish argument, enum on the branch", async () => { + const connected = await connectToPreset(); + const { tools } = await connected.listAllTools(); + + const tool = tools.find((entry) => entry.name === "record_shipment"); + expect(tool).toBeDefined(); + + const schema = toFormSchema(tool?.inputSchema); + const properties = schema?.properties ?? {}; + expect(Object.keys(properties).sort()).toEqual([ + "direction", + "express", + "quantity", + "reference", + ]); + + // The premise of the whole fix: no top-level `type`, and the `enum` sits on + // the surviving branch rather than beside it. + const direction = properties.direction; + expect(direction?.type).toBeUndefined(); + expect(direction?.enum).toBeUndefined(); + expect(direction?.anyOf).toHaveLength(2); + expect(direction?.anyOf).toEqual( + expect.arrayContaining([expect.objectContaining({ type: "null" })]), + ); + }); + + it("collapses each argument to the type its widget dispatches on", async () => { + const connected = await connectToPreset(); + const { tools } = await connected.listAllTools(); + const schema = toFormSchema( + tools.find((entry) => entry.name === "record_shipment")?.inputSchema, + ); + const properties = schema?.properties ?? {}; + + const collapsed = Object.fromEntries( + Object.entries(properties).map(([name, propertySchema]) => [ + name, + normalizeNullableUnion(propertySchema), + ]), + ); + + expect(collapsed.direction?.type).toBe("string"); + expect(collapsed.direction?.enum).toEqual(["envio", "recebimento"]); + expect(collapsed.reference?.type).toBe("string"); + expect(collapsed.quantity?.type).toBe("integer"); + expect(collapsed.express?.type).toBe("boolean"); + + for (const propertySchema of Object.values(collapsed)) { + expect(propertySchema?.nullable).toBe(true); + expect(propertySchema?.anyOf).toBeUndefined(); + } + }); +}); diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 4a69409c..8f61841c 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -315,6 +315,22 @@ describe("hasMissingRequiredFields", () => { expect(hasMissingRequiredFields(schema, { name: null })).toBe(true); }); + // The renderer's collapse only handles a two-member union, but null admission + // has no such limit — this one renders through the JSON fallback, where the + // user can still type `null`, and the schema plainly accepts it. + it("accepts an explicit null for a required three-member union", () => { + const wide: InspectorFormSchema = { + type: "object", + properties: { + mixed: { + anyOf: [{ type: "string" }, { type: "number" }, { type: "null" }], + }, + }, + required: ["mixed"], + }; + expect(hasMissingRequiredFields(wide, { mixed: null })).toBe(false); + }); + it("rejects null for a required field the schema does not describe", () => { const undescribed: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 2cdb2331..093adbc9 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -1,4 +1,7 @@ -import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { + admitsNull, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; export type JsonValue = | string @@ -147,6 +150,12 @@ export function collectSchemaDefaults( * since #1928 the user can produce exactly that by clearing a nullable enum. * Treating it as missing would leave the form's submit permanently disabled on * a value the schema calls valid. + * + * The test is `admitsNull`, **not** whether the renderer's collapse recognized + * the field. Those differ: the collapse only handles a two-member union, so a + * three-member `anyOf: [string, number, null]` renders through the JSON + * fallback — where a user can still type `null` — while plainly admitting it. + * Gating on the collapse would reject a value the schema accepts. */ export function hasMissingRequiredFields( schema: InspectorFormSchema, @@ -158,9 +167,7 @@ export function hasMissingRequiredFields( const value = values[field]; if (value === null) { const fieldSchema = properties[field]; - return fieldSchema === undefined - ? true - : normalizeNullableUnion(fieldSchema).nullable !== true; + return fieldSchema === undefined ? true : !admitsNull(fieldSchema); } return value === undefined || value === ""; }); diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index ae26b042..e17b8508 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -42,6 +42,10 @@ export interface NullableUnionSchema { type?: string | string[]; enum?: unknown[]; anyOf?: readonly unknown[]; + // Read only by `admitsNull`, which considers encodings the collapse itself + // declines to flatten. + oneOf?: readonly unknown[]; + nullable?: boolean; } /** Narrow an `anyOf` member to a readable object, or `null` if it isn't one. */ @@ -120,6 +124,46 @@ function collapsed( } as NormalizedNullableUnion; } +/** + * Whether a schema permits an explicit `null`. + * + * Deliberately **independent of {@link normalizeNullableUnion}**, which is a + * *renderer* question — "can this become one widget?" — and is therefore + * narrower on purpose: it only collapses a two-member union. Null admission is + * a *validity* question and has no such limit, so + * `anyOf: [{ type: "string" }, { type: "number" }, { type: "null" }]` admits + * null even though it renders through the JSON fallback. Deriving one from the + * other would make a form reject a value its own schema accepts, which is why + * these are two functions rather than a `.nullable` flag read off the collapse. + * + * Recognizes every encoding the collapse does, plus the ones it declines: + * `nullable: true`, `type: "null"`, `type: [..., "null"]`, and a `"null"` + * branch anywhere in an `anyOf` or `oneOf` of any size. + */ +export function admitsNull(schema: NullableUnionSchema): boolean { + if (schema.nullable === true) { + return true; + } + if (schema.type === "null") { + return true; + } + if (Array.isArray(schema.type) && schema.type.includes("null")) { + return true; + } + return [schema.anyOf, schema.oneOf].some((branches) => + branches?.some((entry) => { + const branch = toBranch(entry); + if (branch === null) { + return false; + } + return ( + branch.type === "null" || + (Array.isArray(branch.type) && branch.type.includes("null")) + ); + }), + ); +} + /** * Collapse a nullable union — what Zod's `.nullish()` / `.nullable()` and * FastMCP's optional arguments emit — into `{ type: , nullable: true }`.