From 80eb321b1436fb324714984e68f228e305b1f9f1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:49:09 -0400 Subject: [PATCH] fix: stop const-less anyOf branches from crashing the form, and never select a non-string enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2007. Two things, both surfaced while answering "does #1928's PR also fix #2007?". **#2007, and why it belongs in this PR.** `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` whose branches carry no top-level `const`, so every MultiSelect option was `String(item.const ?? "")` — the empty string, twice. Mantine *throws* on duplicate option values, greying out the whole tool panel. That is a pre-existing bug, but this branch makes it reachable from a new direction: a *nullable* array of object unions previously had no top-level `type` and fell to the JSON fallback, and now collapses to `type: "array"` with `items.anyOf` — straight into the crash. Shipping the collapse without this fix would widen an existing crash, so the two go together. Both shapes are covered by tests. `toConstOptions` now returns `null` unless every branch yields a distinct, non-empty value, and the field falls through to the JSON editor — which is the honest widget for a union of object shapes anyway. Applied to the `string`/`oneOf` path as well, which had the identical `?? ""` and the same duplicate-throw waiting on it. **Copilot round 2.** Round 1's fix kept "an explicit `type` is authoritative", so `{ type: "number", enum: [1, 2] }` still collapses with its enum hoisted. The TUI's dispatcher tests `property.enum` *before* the type switch, and `toSelectOptions` stringifies, so that reached a select that submits `"1"` for `1`. The select path is now gated on `isStringEnum` (exported from core for this), and a typed non-string enum falls through to its typed field: the enum constraint is lost, the value's type is not, which is the safer of the two losses. Pre-existing for a plain numeric enum; the collapse just made the nullable form reachable too. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 33 ++++++ clients/tui/src/utils/schemaToForm.ts | 16 ++- .../groups/SchemaForm/SchemaForm.test.tsx | 102 ++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 63 +++++++++-- core/json/nullableUnion.ts | 9 +- 5 files changed, 205 insertions(+), 18 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 93ca1ca9..769c3026 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -311,6 +311,39 @@ describe("schemaToForm", () => { ]); }); + // ink-form hands a select's stringified value straight back, so a numeric + // enum routed to a select would submit "1" for 1. The typed field loses the + // enum constraint but keeps the value's type — the safer loss. + it("routes a nullable numeric enum to its typed field, not a select", () => { + const form = schemaToForm( + { + properties: { + level: { + anyOf: [{ type: "integer", enum: [1, 2] }, { type: "null" }], + }, + ratio: { + anyOf: [{ type: "number", enum: [0.5, 1.5] }, { type: "null" }], + }, + }, + }, + "numericEnum", + ); + expect( + form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })), + ).toEqual([ + { name: "level", type: "integer" }, + { name: "ratio", type: "float" }, + ]); + }); + + it("routes a plain numeric enum to its typed field too", () => { + const form = schemaToForm( + { properties: { level: { type: "integer", enum: [1, 2] } } }, + "plainNumericEnum", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ type: "integer" }); + }); + it("leaves a union of two real types as a plain string field", () => { const form = schemaToForm( { diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 5201abbb..6332450c 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -3,7 +3,10 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; -import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; /** Minimal JSON Schema property shape used when building tool parameter forms */ interface JsonSchemaProperty { @@ -105,12 +108,17 @@ export function schemaToForm( ...baseField, options: toSelectOptions(property.items.enum, property.items.enumNames), } as FormField; - } else if (property.enum) { - // Single select + } else if (isStringEnum(property.enum)) { + // Single select. Gated on the members being strings because + // `toSelectOptions` stringifies them and ink-form hands the string + // straight back: a numeric `enum: [1, 2]` would submit `"1"` and violate + // the schema. A typed non-string enum falls through to its typed field + // below, which loses the enum constraint but keeps the value's type — + // the safer of the two losses. field = { type: "select", ...baseField, - options: toSelectOptions(property.enum, property.enumNames), + options: toSelectOptions(property.enum ?? [], property.enumNames), } as FormField; } else { // Map JSON Schema types to ink-form types diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 3ff41b50..e3bc2b3c 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -971,6 +971,108 @@ describe("SchemaForm nullable unions", () => { expect(onChange).toHaveBeenCalledWith({ profile: { nick: "z" } }); }); + // #2007: `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` + // whose branches carry no top-level `const`, so every MultiSelect option was + // the empty string — and Mantine *throws* on duplicate option values, greying + // out the whole tool panel. The nullable form below is reachable only because + // this PR now collapses it into `type: "array"`, so it has to be safe too. + it("falls back to the JSON input for an array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [objectBranch("A"), objectBranch("B")] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to the JSON input for a nullable array of object-union items", () => { + const objectBranch = (name: string): InspectorFormSchema => ({ + type: "object", + properties: { type: { type: "string", const: name } }, + required: ["type"], + }); + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + anyOf: [ + { type: "array", items: { anyOf: [objectBranch("A")] } }, + { type: "null" }, + ], + }, + }, + }; + 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", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "dup" }, { const: "dup" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA"); + }); + + it("falls back to a text input for a string oneOf with no consts", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + mode: { + title: "Mode", + type: "string", + oneOf: [{ type: "string" }, { type: "string" }], + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Mode/).tagName).toBe("INPUT"); + }); + + it("still renders a MultiSelect when every anyOf branch has a distinct const", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + items: { + title: "Items", + type: "array", + items: { anyOf: [{ const: "a", title: "Alpha" }, { const: "b" }] }, + }, + }, + }; + renderWithMantine( + , + ); + expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument(); + }); + it("still falls back to the JSON input for a union of two real types", () => { 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 628f2ee3..9c203901 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -11,7 +11,10 @@ import { import { useState } from "react"; import { ClearButton } from "../../elements/ClearButton/ClearButton"; import { useValueChange } from "../../../hooks/useValueChange"; -import type { InspectorFormSchema } from "../../../utils/jsonUtils"; +import type { + InspectorFormSchema, + JsonSchemaConst, +} from "../../../utils/jsonUtils"; import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ @@ -52,6 +55,44 @@ function toEnumData( return values; } +/** + * Build `Select`/`MultiSelect` option data from a list of `oneOf`/`anyOf` + * 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: + * + * - **No `const`.** 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 + * whole tool panel rather than degrading (#2007). + * - **Duplicate values.** Two branches sharing a `const` throw the same way. + * - **An empty option value**, which Mantine cannot render as selectable. + * + * A union of object shapes has no faithful dropdown anyway, so the JSON editor + * is the honest widget for it rather than a lossy or crashing one. + */ +function toConstOptions( + branches: (InspectorFormSchema | JsonSchemaConst)[], +): { value: string; label: string }[] | null { + const options: { value: string; label: string }[] = []; + const seen = new Set(); + for (const branch of branches) { + if (branch.const === undefined || branch.const === null) { + return null; + } + const value = String(branch.const); + if (value === "" || seen.has(value)) { + return null; + } + seen.add(value); + options.push({ value, label: branch.title ?? value }); + } + return options.length > 0 ? options : null; +} + /** * Whether an enum `Select` should offer a clear affordance. Only a schema that * admits `null` does: clearing emits `null`, and an enum without a null branch @@ -248,11 +289,11 @@ export function SchemaForm({ } // string with oneOf - if (fieldSchema.type === "string" && fieldSchema.oneOf) { - const data = fieldSchema.oneOf.map((item) => ({ - value: String(item.const ?? ""), - label: item.title ?? String(item.const ?? ""), - })); + const oneOfData = fieldSchema.oneOf + ? toConstOptions(fieldSchema.oneOf) + : null; + if (fieldSchema.type === "string" && oneOfData) { + const data = oneOfData; return (