From 274a328abef59744878e15b3d04aad7c3b5bc412 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 19:58:07 -0400 Subject: [PATCH] fix: apply the fail-closed rule uniformly, and correct the admitsNull docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 9: four findings, all of them places where round 8's fail-closed inversion was the right idea applied incompletely. The rule was only enforced at the positions I happened to be editing; it now holds everywhere a schema is inspected. 1. `admitsNull` returned early on an explicit `type`, ignoring a sibling `anyOf` — but the two are conjunctive, so `{ type: ["string", "null"], anyOf: [{ type: "string" }] }` admits no null at all. A sibling union now withholds the claim. 2. A null-typed branch could be unsatisfiable via its own nested `anyOf` (`{ type: "null", anyOf: [{ type: "string" }] }`). The branch check tested `not`/`allOf`/`oneOf` but not `anyOf`; it now treats a nested union as equally opaque. 3. The `type: [T, "null"]` collapse path had no applicator guard at all — `collapsed` clears `anyOf` unconditionally and the renderers ignore the rest, so `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` was widened into an unconstrained string field. That path now declines the same way the `anyOf` path does. 4. The `admitsNull` docstring still advertised `oneOf` support that round 8 deliberately removed. Rewritten to state what is recognized, what is declined and why, and the asymmetry that settles the direction. The three code fixes share one helper (`hasOpaqueApplicator`) rather than repeating the condition, so the next position that needs it cannot drift. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 49 ++++++++++++-- core/json/nullableUnion.ts | 66 +++++++++++++++---- 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index 3a59953a..525dae29 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -357,6 +357,26 @@ describe("normalizeNullableUnion", () => { }); }); + // 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. + it("declines a type-array nullable that also carries a sibling anyOf", () => { + const schema = { + type: ["string", "null"], + anyOf: [{ const: "a" }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("declines a type-array nullable that carries an opaque applicator", () => { + const withNot = { type: ["string", "null"], not: { const: "a" } }; + expect(normalizeNullableUnion(withNot)).toBe(withNot); + const withOneOf = { type: ["string", "null"], oneOf: [{ const: "a" }] }; + expect(normalizeNullableUnion(withOneOf)).toBe(withOneOf); + const withAllOf = { type: ["string", "null"], allOf: [{ minLength: 2 }] }; + expect(normalizeNullableUnion(withAllOf)).toBe(withAllOf); + }); + it("normalizes type array array|null", () => { expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({ type: "array", @@ -524,6 +544,17 @@ describe("admitsNull", () => { ).toBe(false); }); + it("ignores a null branch made unsatisfiable by its own nested anyOf", () => { + expect( + admitsNull({ + anyOf: [ + { type: "string" }, + { type: "null", anyOf: [{ type: "string" }] }, + ], + }), + ).toBe(false); + }); + it("still accepts a plain null branch alongside an unsatisfiable one", () => { expect( admitsNull({ @@ -588,14 +619,20 @@ describe("admitsNull", () => { }); it("still honors an explicit type that does admit null", () => { + expect(admitsNull({ type: ["string", "null"] })).toBe(true); + expect(admitsNull({ type: "null" })).toBe(true); + }); + + // A sibling union is conjunctive with the `type`, so it can reject null even + // when the type list names it. It is not evaluated here, so its mere presence + // withholds the claim. + it("withholds the claim when a sibling anyOf sits beside an explicit type", () => { expect( - admitsNull({ - type: ["string", "null"], - anyOf: [{ type: "string" }, { type: "null" }], - }), - ).toBe(true); + admitsNull({ type: ["string", "null"], anyOf: [{ type: "string" }] }), + ).toBe(false); + // Unsatisfiable: names null, admits nothing. expect(admitsNull({ type: "null", anyOf: [{ type: "string" }] })).toBe( - true, + false, ); }); diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 9f79af3d..2e4db037 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -188,6 +188,23 @@ function typeNamesNull(type: unknown): boolean { return Array.isArray(type) ? type.includes("null") : type === "null"; } +/** + * Applicators this module does not evaluate, and which are **conjunctive** with + * everything beside them — so any of them can rule `null` out, or constrain a + * value in a way no widget here would honor. + * + * `anyOf` is excluded because it is the keyword being *recognized*: a caller + * that has already identified the nullable-union shape checks it directly. Every + * other position treats a stray `anyOf` as opaque too. + */ +function hasOpaqueApplicator(schema: NullableUnionSchema): boolean { + return ( + schema.not !== undefined || + schema.allOf !== undefined || + schema.oneOf !== undefined + ); +} + /** * What {@link stripNullEnumMembers} concluded about an `enum`. * @@ -301,9 +318,24 @@ function collapsed( * 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. + * **Answers `false` for anything it cannot fully evaluate**, which is a larger + * set than it may look. Recognized: `nullable: true`, `type: "null"`, + * `type: [..., "null"]`, and a `"null"` branch in an `anyOf` of any size. + * Deliberately *not* recognized, despite naming null: + * + * - **`oneOf`** — it requires *exactly one* branch to match, so a `null` branch + * does not mean `null` validates; two matching branches make it fail. + * - **`not` / `allOf`**, at this level or on the branch, which can rule null out. + * - **A sibling `anyOf` beside an explicit `type`**, since the two are + * conjunctive and the union is not evaluated here. + * - **A branch that names null but is unsatisfiable**, whether by its own + * `enum`/`const` or its own nested applicators. + * + * The asymmetry is what settles the direction: under-claiming costs a clear + * button and treats a valid `null` as missing, while over-claiming lets the + * form emit a value the schema rejects. So callers get a firm "no" rather than + * an optimistic guess, and a schema this module cannot read renders through the + * JSON editor with its constraints intact. */ export function admitsNull(schema: NullableUnionSchema): boolean { if (nullExcludedBySiblings(schema)) { @@ -315,11 +347,7 @@ export function admitsNull(schema: NullableUnionSchema): boolean { // `null` validates — two matching branches make it fail. Rather than answer // these half-correctly, say no: under-claiming nullability costs a clear // button, while over-claiming lets the form emit a value the schema rejects. - if ( - schema.not !== undefined || - schema.allOf !== undefined || - schema.oneOf !== undefined - ) { + if (hasOpaqueApplicator(schema)) { return false; } if (schema.nullable === true) { @@ -331,6 +359,13 @@ export function admitsNull(schema: NullableUnionSchema): boolean { // as the union. Falling through to the branch scan here would let the union // override a constraint that outranks it. if (schema.type !== undefined) { + // A sibling `anyOf` is conjunctive with the `type`, so it can reject null + // even when the type list names it — `{ type: ["string", "null"], anyOf: + // [{ type: "string" }] }` admits no null at all. Not evaluated here, so + // its mere presence withholds the claim. + if (schema.anyOf !== undefined) { + return false; + } return typeNamesNull(schema.type); } return ( @@ -345,9 +380,10 @@ export function admitsNull(schema: NullableUnionSchema): boolean { const branchSchema = branch as NullableUnionSchema; return ( !nullExcludedBySiblings(branchSchema) && - branchSchema.not === undefined && - branchSchema.allOf === undefined && - branchSchema.oneOf === undefined + !hasOpaqueApplicator(branchSchema) && + // A nested union inside the branch is as opaque as the others: `{ type: + // "null", anyOf: [{ type: "string" }] }` names null and admits nothing. + branchSchema.anyOf === undefined ); }) ?? false ); @@ -408,7 +444,13 @@ export function normalizeNullableUnion( if ( Array.isArray(schema.type) && schema.type.length === 2 && - schema.type.includes("null") + schema.type.includes("null") && + // This path keeps the schema's own keywords, but `collapsed` clears `anyOf` + // and the renderers ignore the other applicators — so a compound schema + // like `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` would be + // widened into an unconstrained string field. Leave it for the JSON editor. + schema.anyOf === undefined && + !hasOpaqueApplicator(schema) ) { const type = schema.type.find((member) => member !== "null"); if (isRenderableType(type)) {