From 562709a79a4c4d59d344a89261c59646bb5b2a56 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 20:11:59 -0400 Subject: [PATCH] fix: guard the surviving branch's own composition, via one shared predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 10: one finding, the last position of the rule the previous two rounds established. A *non-null* branch can carry its own `anyOf`, and the hoist drops it — the branch's `anyOf` overwrites the wrapper's in the spread and is then cleared, so `{ anyOf: [{ type: "string", anyOf: [{ const: "a" }] }, { type: "null" }] }` widened into an unconstrained nullable string. Rather than add a fourth inline condition, the four sites that ask "does this schema compose something I cannot evaluate?" now share one predicate, `hasUnevaluatedComposition` (an opaque applicator, or a nested `anyOf`): - the surviving branch in the `anyOf` collapse (this round's finding), - the null branch in `admitsNull`, - the `type: [T, "null"]` collapse path, - and, via `hasOpaqueApplicator`, the top of `admitsNull`. Each of those was missed independently across rounds 8-10 for the same reason: the guard existed but was spelled out inline at each site and drifted. Sharing the predicate is the part that stops a fifth site from repeating it. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 37 +++++++++++++++++ core/json/nullableUnion.ts | 40 +++++++++++++++---- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index 525dae29..2f125a82 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -450,6 +450,43 @@ describe("normalizeNullableUnion", () => { expect(normalizeNullableUnion(schema)).toBe(schema); }); + // The hoist *drops* a nested union: the branch's `anyOf` overwrites the + // wrapper's in the spread and is then cleared, so the constraint vanishes and + // the field widens into an unconstrained nullable string. + it("declines when the surviving branch carries its own anyOf", () => { + const schema = { + anyOf: [ + { type: "string", anyOf: [{ const: "a" }] }, + { type: "null" as const }, + ], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("declines when the surviving branch carries an opaque applicator", () => { + const withNot = { + anyOf: [ + { type: "string", not: { const: "a" } }, + { type: "null" as const }, + ], + }; + expect(normalizeNullableUnion(withNot)).toBe(withNot); + const withAllOf = { + anyOf: [ + { type: "string", allOf: [{ minLength: 2 }] }, + { type: "null" as const }, + ], + }; + expect(normalizeNullableUnion(withAllOf)).toBe(withAllOf); + const withOneOf = { + anyOf: [ + { type: "string", oneOf: [{ const: "a" }] }, + { type: "null" as const }, + ], + }; + expect(normalizeNullableUnion(withOneOf)).toBe(withOneOf); + }); + it("declines when the wrapper carries an applicator the module cannot read", () => { const schema = { allOf: [{ minLength: 2 }], diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 2e4db037..7db747f4 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -205,6 +205,26 @@ function hasOpaqueApplicator(schema: NullableUnionSchema): boolean { ); } +/** + * Whether a schema composes further constraints this module cannot evaluate — + * {@link hasOpaqueApplicator}, **plus a nested `anyOf`**. + * + * This is the single test every position uses on a schema that is *not* the + * union being recognized: the surviving branch, a null branch, and the + * `type: [T, "null"]` schema. Each of those was missed independently at some + * point — the guard existed but was spelled out inline and drifted between + * sites — so they now share one predicate rather than three conditions that + * have to be kept in step. + * + * A nested `anyOf` matters as much as the others because the hoist *drops* it: + * the branch's `anyOf` overwrites the wrapper's in the spread and is then + * cleared, so `{ anyOf: [{ type: "string", anyOf: [...] }, { type: "null" }] }` + * would otherwise widen into an unconstrained nullable string. + */ +function hasUnevaluatedComposition(schema: NullableUnionSchema): boolean { + return schema.anyOf !== undefined || hasOpaqueApplicator(schema); +} + /** * What {@link stripNullEnumMembers} concluded about an `enum`. * @@ -378,12 +398,12 @@ export function admitsNull(schema: NullableUnionSchema): boolean { // const: "x" }` is unsatisfiable, and its own applicators are as opaque // here as the wrapper's. const branchSchema = branch as NullableUnionSchema; + // A branch that names null can still admit nothing: `{ type: "null", + // const: "x" }` is unsatisfiable, and a nested union or applicator inside + // it is as opaque here as one on the wrapper. return ( !nullExcludedBySiblings(branchSchema) && - !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 + !hasUnevaluatedComposition(branchSchema) ); }) ?? false ); @@ -424,7 +444,14 @@ export function normalizeNullableUnion( // Hoisting would silently drop a wrapper constraint the branch also // carries, so decline instead; see `branchDropsWrapperConstraint`. - if (nullBranch && branch && !wrapperCarriesConstraints(schema)) { + // The wrapper may only carry annotations, and the surviving branch may not + // compose anything this module cannot evaluate — the hoist would drop it. + if ( + nullBranch && + branch && + !wrapperCarriesConstraints(schema) && + !hasUnevaluatedComposition(branch as NullableUnionSchema) + ) { // A branch may carry an `enum` and no `type`; JSON Schema allows that, and // an all-string enum is unambiguously a string field. See isStringEnum for // why a non-string enum deliberately does not get the same treatment. @@ -449,8 +476,7 @@ export function normalizeNullableUnion( // 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) + !hasUnevaluatedComposition(schema) ) { const type = schema.type.find((member) => member !== "null"); if (isRenderableType(type)) {