From 8356033d58b29c219c256f701e92d5b3c57a4832 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:28:38 -0400 Subject: [PATCH] fix: strip the enum null sentinel, and make the preset test cover its wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 4: no new visible comments, two in the suppressed block. Both valid. 1. The `type: [T, "null"]` encoding keeps its keywords at the top level, so a nullable enum written that way carries the null *inside* the list: `{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. The collapse left it there, which broke both renderers in different ways — the web dispatcher handed `null` to Mantine as option data, and the TUI's all-strings check rejected the whole enum and fell back to a plain text field. Neither is the dropdown the schema asks for. The collapse now strips a `null` member whenever it flattens, since that fact has already moved onto `nullable`. An enum of nothing but `null` drops out entirely: it offers no value a dropdown could show, so the plain widget serves the field better than an empty select. Covered in core and end-to-end in both dispatchers. 2. The new integration test claimed in its own header to pin the registry and config wiring, then called `createNullableFieldsTool()` directly — so a misspelt preset case or a config naming a dead preset would have left it green. A fair "this does not test what it says" catch. It now resolves the checked-in `nullable-fields-http.json` through `loadConfig` → `resolveConfig`, matching `duplicate-tool-names.test.ts`, and asserts the resolved tool names before booting the server. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 23 ++++++ .../groups/SchemaForm/SchemaForm.test.tsx | 28 +++++++ .../web/src/test/core/nullableUnion.test.ts | 58 +++++++++++++++ .../integration/mcp/nullable-fields.test.ts | 74 +++++++++++++------ core/json/nullableUnion.ts | 33 ++++++++- 5 files changed, 190 insertions(+), 26 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 769c3026..f47450e3 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -293,6 +293,29 @@ describe("schemaToForm", () => { }); }); + // With the null left in the list, the all-strings check would reject the + // enum and this would degrade to a plain text field. + it("renders a select for a type: [string, null] enum, null stripped", () => { + const form = schemaToForm( + { + properties: { + direction: { + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }, + }, + }, + "typeArrayNullableEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ + type: "select", + options: [ + { label: "envio", value: "envio" }, + { label: "recebimento", value: "recebimento" }, + ], + }); + }); + it("collapses the type: [T, null] encoding too", () => { const form = schemaToForm( { diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 287da67b..0110bf00 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -886,6 +886,34 @@ describe("SchemaForm nullable unions", () => { ).not.toBeInTheDocument(); }); + // The other supported nullable encoding: keywords stay at the top level, so + // the null sentinel sits inside the enum list rather than on a branch. + it("renders a Select for a type: [string, null] enum, without a null option", async () => { + const user = userEvent.setup(); + const onChange = vi.fn(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento", null] as unknown as string[], + }, + }, + }; + renderWithMantine( + , + ); + await user.click(screen.getByRole("textbox", { name: "Direction" })); + const options = await screen.findAllByRole("option", { hidden: true }); + expect(options.map((option) => option.textContent)).toEqual([ + "envio", + "recebimento", + ]); + await user.click(options[0]); + expect(onChange).toHaveBeenCalledWith({ direction: "envio" }); + }); + it("renders a TextInput for a type: [string, null] field", async () => { const user = userEvent.setup(); const onChange = vi.fn(); diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index 485febf1..2bf7a1c9 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -246,6 +246,64 @@ describe("normalizeNullableUnion", () => { }); }); + // The `type: [T, "null"]` encoding keeps its keywords at the top level, so a + // nullable enum written that way carries the null *inside* the list. Leaving + // it there hands `null` to Mantine as option data, and makes the TUI's + // all-strings check reject the enum and fall back to a text field. + it("strips the null sentinel from a type-array nullable enum", () => { + expect( + normalizeNullableUnion({ + type: ["string", "null"], + enum: ["envio", "recebimento", null], + }), + ).toEqual({ + type: "string", + enum: ["envio", "recebimento"], + anyOf: undefined, + nullable: true, + }); + }); + + it("strips a null sentinel hoisted off an anyOf branch too", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "string", enum: ["a", null] }, { type: "null" }], + }), + ).toEqual({ + type: "string", + enum: ["a"], + anyOf: undefined, + nullable: true, + }); + }); + + it("drops an enum that held nothing but null", () => { + // No value a dropdown could offer, so the field is better served by its + // plain widget than by an empty select. + expect( + normalizeNullableUnion({ type: ["string", "null"], enum: [null] }), + ).toEqual({ + type: "string", + enum: undefined, + anyOf: undefined, + nullable: true, + }); + }); + + it("leaves a null-free enum untouched", () => { + expect( + normalizeNullableUnion({ + type: ["string", "null"], + enum: ["a", "b"], + }), + ).toEqual({ + type: "string", + enum: ["a", "b"], + anyOf: undefined, + nullable: true, + }); + }); + it("normalizes type array array|null", () => { expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({ type: "array", diff --git a/clients/web/src/test/integration/mcp/nullable-fields.test.ts b/clients/web/src/test/integration/mcp/nullable-fields.test.ts index 0a54e142..6396a85b 100644 --- a/clients/web/src/test/integration/mcp/nullable-fields.test.ts +++ b/clients/web/src/test/integration/mcp/nullable-fields.test.ts @@ -1,35 +1,47 @@ 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 { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; import { toFormSchema } from "../../../utils/jsonUtils"; +import type { InspectorFormSchema } from "../../../utils/jsonUtils"; import { createTestServerHttp, type TestServerHttp, createTestServerInfo, - createNullableFieldsTool, + loadConfig, + resolveConfig, } 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. + * Live coverage of `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. + * nothing else pins it — a Zod change in how it compiles a nullish enum 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. + * So the server is built by **resolving the checked-in config**, not by calling + * the fixture factory directly. That is deliberate and is the difference + * between covering the wiring and merely asserting the factory: going through + * `loadConfig` → `resolveConfig` means a misspelt preset name in + * `preset-registry.ts`, or a config that names a preset that no longer exists, + * fails here rather than only when someone runs the repro by hand. */ describe("nullable argument schemas over the wire (#1928)", () => { let client: InspectorClient | null = null; let server: TestServerHttp | null = null; + const configPath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs/nullable-fields-http.json", + ); + afterEach(async () => { if (client) { try { @@ -49,10 +61,16 @@ describe("nullable argument schemas over the wire (#1928)", () => { } }); - async function connectToPreset(): Promise { + /** + * Boot the showcase config. The harness picks the port rather than using the + * config's fixed one, so this cannot collide with a showcase server someone + * is running by hand. + */ + async function connectToShowcase(): Promise { + const resolved = resolveConfig(loadConfig(configPath)); const started = createTestServerHttp({ serverInfo: createTestServerInfo("nullable-fields-test", "1.0.0"), - tools: [createNullableFieldsTool()], + tools: resolved.tools, }); await started.start(); server = started; @@ -66,15 +84,29 @@ describe("nullable argument schemas over the wire (#1928)", () => { return connected; } - it("emits an anyOf-with-null for every nullish argument, enum on the branch", async () => { - const connected = await connectToPreset(); + async function recordShipmentSchema(): Promise { + const connected = await connectToShowcase(); 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(schema).not.toBeNull(); + return schema as InspectorFormSchema; + } + + it("resolves the preset the config names", () => { + // Fails on a misspelt registry case or a config naming a dead preset — + // neither of which the hand-built unit schemas can see. + const resolved = resolveConfig(loadConfig(configPath)); + expect(resolved.tools?.map((tool) => tool.name)).toEqual([ + "record_shipment", + "get_temp", + ]); + }); + + it("emits an anyOf-with-null for every nullish argument, enum on the branch", async () => { + const schema = await recordShipmentSchema(); + const properties = schema.properties ?? {}; expect(Object.keys(properties).sort()).toEqual([ "direction", "express", @@ -94,15 +126,9 @@ describe("nullable argument schemas over the wire (#1928)", () => { }); 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 schema = await recordShipmentSchema(); const collapsed = Object.fromEntries( - Object.entries(properties).map(([name, propertySchema]) => [ + Object.entries(schema.properties ?? {}).map(([name, propertySchema]) => [ name, normalizeNullableUnion(propertySchema), ]), diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index e17b8508..edcee0c4 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -101,6 +101,30 @@ export type NormalizedNullableUnion = Omit< nullable?: boolean; }; +/** + * Drop a `null` sentinel from an `enum`, since the collapse has already moved + * that fact onto `nullable`. + * + * The `type: [T, "null"]` encoding keeps its keywords at the top level, so a + * nullable enum written that way carries the null *inside* the list: + * `{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. Leaving + * it there breaks both renderers in different ways — the web dispatcher would + * hand `null` to Mantine as option data, and the TUI's all-strings check would + * reject the whole enum and fall back to a plain text field. Neither is the + * dropdown the schema is asking for. + * + * Returns `undefined` when nothing but `null` was in the list: an enum of only + * `null` permits no value a dropdown could offer, so the field is better served + * by its plain widget than by an empty select. + */ +function stripNullFromEnum(value: unknown): unknown[] | undefined { + if (!Array.isArray(value)) { + return undefined; + } + const members = value.filter((member) => member !== null); + return members.length > 0 ? members : undefined; +} + /** * Build the collapsed schema. * @@ -115,12 +139,17 @@ function collapsed( branch: Record | undefined, type: RenderableType, ): NormalizedNullableUnion { + const merged = { ...schema, ...branch }; return { - ...schema, - ...branch, + ...merged, type, anyOf: undefined, nullable: true, + // `enum` is read after the spread so the branch's list wins when it has + // one, and the null sentinel is stripped either way. + ...(merged.enum === undefined + ? {} + : { enum: stripNullFromEnum(merged.enum) }), } as NormalizedNullableUnion; }