fix: strip the enum null sentinel, and make the preset test cover its wiring

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 <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 18:28:38 -04:00
commit 8356033d58
3 changed files with 190 additions and 26 deletions

View file

@ -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(
<SchemaForm schema={schema} values={{}} onChange={onChange} />,
);
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();

View file

@ -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",

View file

@ -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: [<branch>, { 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<InspectorClient> {
/**
* 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<InspectorClient> {
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<InspectorFormSchema> {
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),
]),