From 18053d4cb2dfeba7012590b407a809afd302e820 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 20:27:19 -0400 Subject: [PATCH] fix: decline empty enums, drop mislabelling enumNames, treat $ref as opaque MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 11. Two of the three are the enum-strip logic this PR added misbehaving on degenerate input; the third is one more keyword for the fail-closed rule. 1. An empty `enum` permits nothing at all, but the strip reported "unchanged" (nothing was removed) and the collapse built a Select with no options — a required field then had no interaction able to supply a value. It now reaches the same conclusion as `[null]`: no selectable member, so decline. The outcome is renamed `unselectable`, since "only-null" no longer describes both routes to it. 2. Filtering `null` out of `enum` shortens it, which could make an ALREADY mismatched `enumNames` come out the same length — so `enum: [null, "b"]` with `enumNames: ["None"]` started labelling `b` as "None", turning labels both renderers had correctly ignored into a confident mislabel. A list that was not parallel before filtering is now dropped rather than kept. 3. `$ref` joins the opaque set. It is never resolved here and applies alongside its siblings, so `{ type: "string", $ref: "#/$defs/intOnly" }` can be unsatisfiable. The wrapper path already declined on it by virtue of the annotation allowlist; this extends the same rule to branches, the `type: [T, "null"]` path, and `admitsNull`. No round 12 was requested: the review loop was stopped at the user's direction. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 55 ++++++++++++++++++- core/json/nullableUnion.ts | 53 +++++++++++------- 2 files changed, 85 insertions(+), 23 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index 2f125a82..eb22ab2e 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -341,7 +341,27 @@ describe("normalizeNullableUnion", () => { }); }); - it("leaves a mismatched enumNames alone rather than inventing an alignment", () => { + // Keeping a mismatched list is not neutral: filtering shortens `enum`, so a + // list both renderers had correctly *ignored* can come out the same length + // and start labelling — `enum: [null, "b"]` + `enumNames: ["None"]` would + // otherwise label `b` as "None". Dropping it preserves the ignore. + it("drops a mismatched enumNames rather than letting filtering align it", () => { + expect( + normalizeNullableUnion({ + type: ["string", "null"], + enum: [null, "b"], + enumNames: ["None"], + }), + ).toEqual({ + type: "string", + enum: ["b"], + enumNames: undefined, + anyOf: undefined, + nullable: true, + }); + }); + + it("drops a mismatched enumNames that stays mismatched too", () => { expect( normalizeNullableUnion({ type: ["string", "null"], @@ -351,12 +371,43 @@ describe("normalizeNullableUnion", () => { ).toEqual({ type: "string", enum: ["a"], - enumNames: ["Alpha"], + enumNames: undefined, anyOf: undefined, nullable: true, }); }); + // An empty enum permits nothing at all, so it reaches the same conclusion as + // `[null]`: no value a dropdown could offer, so do not build one. + it("declines to collapse an empty enum", () => { + const typeArray = { type: ["string", "null"], enum: [] }; + expect(normalizeNullableUnion(typeArray)).toBe(typeArray); + const union = { + anyOf: [{ type: "string", enum: [] }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(union)).toBe(union); + }); + + // `$ref` applies alongside its siblings and is never resolved here, so a + // branch carrying one can be unsatisfiable. + it("treats $ref as opaque wherever it appears", () => { + const onBranch = { + anyOf: [ + { type: "string", $ref: "#/$defs/intOnly" }, + { type: "null" as const }, + ], + }; + expect(normalizeNullableUnion(onBranch)).toBe(onBranch); + const onTypeArray = { type: ["string", "null"], $ref: "#/$defs/intOnly" }; + expect(normalizeNullableUnion(onTypeArray)).toBe(onTypeArray); + expect(admitsNull({ type: ["string", "null"], $ref: "#/$defs/x" })).toBe( + false, + ); + expect(admitsNull({ anyOf: [{ type: "null", $ref: "#/$defs/x" }] })).toBe( + false, + ); + }); + // The type-array path keeps the schema's own keywords, but `collapsed` clears // `anyOf` and the renderers ignore the other applicators — so a compound // schema would be widened into an unconstrained field rather than collapsed. diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 7db747f4..e4cc8b24 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -47,6 +47,7 @@ export interface NullableUnionSchema { oneOf?: readonly unknown[]; allOf?: readonly unknown[]; not?: unknown; + $ref?: string; nullable?: boolean; const?: unknown; } @@ -201,7 +202,12 @@ function hasOpaqueApplicator(schema: NullableUnionSchema): boolean { return ( schema.not !== undefined || schema.allOf !== undefined || - schema.oneOf !== undefined + schema.oneOf !== undefined || + // `$ref` points at a schema this module never resolves, and it applies + // *alongside* its siblings — `{ type: "string", $ref: "#/$defs/intOnly" }` + // can be unsatisfiable. The wrapper path already declines on it (it is not + // an annotation); this is the same rule for every other position. + schema.$ref !== undefined ); } @@ -228,13 +234,13 @@ function hasUnevaluatedComposition(schema: NullableUnionSchema): boolean { /** * What {@link stripNullEnumMembers} concluded about an `enum`. * - * `"only-null"` is a distinct outcome rather than "an empty list" because it + * `"unselectable"` is a distinct outcome rather than "an empty list" because it * has to **stop the collapse**, not just empty a field. See below. */ type EnumStrip = | { kind: "unchanged" } | { kind: "filtered"; members: unknown[]; names?: unknown } - | { kind: "only-null" }; + | { kind: "unselectable" }; /** * Drop `null` members from an `enum`, keeping the parallel `enumNames` aligned. @@ -252,12 +258,13 @@ type EnumStrip = * the two lengths disagree — so stripping one without the other silently loses * every label rather than just the dropped one's. * - * **An enum of nothing but `null` reports `"only-null"` so the caller declines - * to collapse at all.** Emitting `enum: undefined` instead would turn a schema - * permitting *only* `null` into a plain string field that accepts arbitrary - * text — trading a cosmetic problem for a correctness one, since the form would - * then invite values the schema forbids. Left uncollapsed it renders through - * the JSON editor, which represents it honestly. + * **An enum that offers no selectable value reports `"unselectable"`, so the + * caller declines to collapse at all.** That covers `[null]` and the empty + * `[]`: the first permits only `null`, the second permits nothing. Emitting + * `enum: undefined` instead would turn either into a plain string field + * accepting arbitrary text — trading a cosmetic problem for a correctness one, + * since the form would then invite values the schema forbids. Left uncollapsed + * they render through the JSON editor, which represents them honestly. */ function stripNullEnumMembers(members: unknown, names: unknown): EnumStrip { if (!Array.isArray(members)) { @@ -266,19 +273,23 @@ function stripNullEnumMembers(members: unknown, names: unknown): EnumStrip { const kept = members .map((member, index) => ({ member, index })) .filter((entry) => entry.member !== null); + // An empty `enum` permits nothing at all, so it reaches the same conclusion + // as `[null]` by a different route: there is no value a dropdown could offer. + if (kept.length === 0) { + return { kind: "unselectable" }; + } if (kept.length === members.length) { return { kind: "unchanged" }; } - if (kept.length === 0) { - return { kind: "only-null" }; - } - // Only realign names that were positionally parallel to begin with; a - // mismatched list is already ignored by both renderers, so re-indexing it - // would invent an alignment the server never declared. - const aligned = - Array.isArray(names) && names.length === members.length - ? kept.map((entry) => names[entry.index]) - : names; + // Only realign names that were positionally parallel to begin with, and DROP + // them otherwise. Keeping a mismatched list is not neutral: filtering shortens + // `enum`, so `enum: [null, "b"]` with `enumNames: ["None"]` would come out the + // same length and start *labelling* `b` as "None" — turning labels both + // renderers had correctly ignored into a confident mislabel. + const wasParallel = Array.isArray(names) && names.length === members.length; + const aligned = wasParallel + ? kept.map((entry) => names[entry.index]) + : undefined; return { kind: "filtered", members: kept.map((entry) => entry.member), @@ -309,7 +320,7 @@ function collapsed( merged.enum, (merged as { enumNames?: unknown }).enumNames, ); - if (strip.kind === "only-null") { + if (strip.kind === "unselectable") { return null; } return { @@ -458,7 +469,7 @@ export function normalizeNullableUnion( const type = branch.type ?? (isStringEnum(branch.enum) ? "string" : undefined); if (isRenderableType(type)) { - // `null` when the merged enum permits only `null`, which is not + // `null` when the merged enum offers nothing selectable, which is not // collapsible — fall through and return the schema by identity. const result = collapsed(schema, branch, type); if (result !== null) {