From 246ee1c262548dfd67e8cbd406ba29070a71eb29 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 04:07:50 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=209?= =?UTF-8?q?=20=E2=80=94=20canonical=20constant=20comparison,=20name-based?= =?UTF-8?q?=20inference,=20permissive=20schemas?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - rootUnion: schema values are compared through a key-sorted rendering, so two discriminator constants written as {a,b} and {b,a} are recognized as the same value rather than as mutually exclusive alternatives. - selectBranchIndex: after the const and required passes, a name only ONE alternative declares identifies that alternative; a name several declare is ambiguous and says nothing. - additionalProperties counts as restrictive only when its schema states an assertion — an annotation-only `{ title: ... }` is the equivalent of `true`, like `{}`. - Drop the unjustified double casts from the ToolTestModal fixtures; the schemas are assignable to Partial as written. Signed-off-by: cliffhall --- clients/tui/__tests__/ToolTestModal.test.tsx | 4 +- clients/web/src/test/core/rootUnion.test.ts | 56 +++++++++++++++- core/json/rootUnion.ts | 69 ++++++++++++++++---- 3 files changed, 112 insertions(+), 17 deletions(-) diff --git a/clients/tui/__tests__/ToolTestModal.test.tsx b/clients/tui/__tests__/ToolTestModal.test.tsx index a933bc17..63c2100c 100644 --- a/clients/tui/__tests__/ToolTestModal.test.tsx +++ b/clients/tui/__tests__/ToolTestModal.test.tsx @@ -100,7 +100,7 @@ describe("ToolTestModal", () => { }, ], }, - } as unknown as Partial); + }); const api = render( { properties: { a: { type: "string" }, b: { type: "string" } }, required: ["a", "b"], }, - } as unknown as Partial); + }); const api = render( { expect(branches).toEqual([]); }); + it("treats an annotation-only additionalProperties schema as permissive", () => { + // `{ title: … }` asserts nothing, so it is the equivalent of `true` just + // as `{}` is — declining on key count alone would recreate the empty form. + const { branches } = resolveRootUnion({ + type: "object", + additionalProperties: { title: "Extra value" }, + anyOf: [EMAIL, SMS], + }); + expect(branches).toHaveLength(2); + }); + it("treats an empty additionalProperties schema as permissive", () => { // `{}` is the JSON Schema equivalent of `true` — it constrains nothing. const { branches } = resolveRootUnion({ @@ -495,6 +506,25 @@ describe("resolveRootUnion", () => { expect(branches).toHaveLength(2); }); + it("compares object constants irrespective of member order", () => { + // Member order carries no meaning in JSON, so these two alternatives are + // pinned to the SAME value and both match — not mutually exclusive. + const { branches } = resolveRootUnion({ + type: "object", + oneOf: [ + { + type: "object", + properties: { tag: { const: { a: 1, b: 2 } }, x: {} }, + }, + { + type: "object", + properties: { tag: { const: { b: 2, a: 1 } }, y: {} }, + }, + ], + }); + expect(branches).toEqual([]); + }); + it("declines a oneOf whose named discriminator does not distinguish", () => { const { branches } = resolveRootUnion({ type: "object", @@ -680,7 +710,9 @@ describe("resolveRootUnion", () => { expect(selectBranchIndex(ambiguous, { kind: "email" })).toBeNull(); }); - it("reports none for a branch that pins nothing", () => { + it("identifies a branch from a name only it declares", () => { + // Nothing is pinned and nothing is required, but `a` belongs to one + // alternative as plainly as a discriminator would. const unpinned = resolveRootUnion({ type: "object", anyOf: [ @@ -688,7 +720,27 @@ describe("resolveRootUnion", () => { { type: "object", properties: { b: { type: "string" } } }, ], }).branches; - expect(selectBranchIndex(unpinned, { a: "x" })).toBeNull(); + expect(selectBranchIndex(unpinned, { a: "x" })).toBe(0); + expect(selectBranchIndex(unpinned, { b: "x" })).toBe(1); + }); + + it("treats a name several branches declare as saying nothing", () => { + const shared = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { both: { type: "string" }, a: { type: "string" } }, + }, + { + type: "object", + properties: { both: { type: "string" }, b: { type: "string" } }, + }, + ], + }).branches; + expect(selectBranchIndex(shared, { both: "x" })).toBeNull(); + // …and both branches named at once is no answer either. + expect(selectBranchIndex(shared, { a: "x", b: "y" })).toBeNull(); }); }); }); diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 09374b9d..f18fa748 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -171,9 +171,33 @@ const ANNOTATION_KEYWORDS = new Set([ "default", ]); +/** + * A JSON rendering whose object keys are sorted, so two values that differ only + * in the order their properties were written render identically. + * + * Member order carries no meaning in JSON, so `{ a: 1, b: 2 }` and + * `{ b: 2, a: 1 }` are the same value — a plain `JSON.stringify` comparison + * would call them different and, for a discriminator, would report two + * alternatives as mutually exclusive when both accept the same input. + */ +function canonicalJson(value: unknown): string { + if (Array.isArray(value)) { + return `[${value.map(canonicalJson).join(",")}]`; + } + if (typeof value === "object" && value !== null) { + const entries = Object.entries(value as Record) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + .map( + ([key, member]) => `${JSON.stringify(key)}:${canonicalJson(member)}`, + ); + return `{${entries.join(",")}}`; + } + return JSON.stringify(value) ?? "undefined"; +} + /** Structural equality, via canonical JSON — enough for schema keyword values. */ function sameValue(a: unknown, b: unknown): boolean { - return a === b || JSON.stringify(a) === JSON.stringify(b); + return a === b || canonicalJson(a) === canonicalJson(b); } /** @@ -294,7 +318,7 @@ function hasDiscriminator( return property?.const; }); if (constants.some((value) => value === undefined)) return false; - const seen = new Set(constants.map((value) => JSON.stringify(value))); + const seen = new Set(constants.map(canonicalJson)); return seen.size === constants.length; }); } @@ -497,12 +521,15 @@ export function resolveRootUnion( const additional = base.additionalProperties; const restrictsAdditional = additional === false || - // An EMPTY schema object is the JSON Schema equivalent of `true` — it - // constrains nothing, so it is no reason to decline. Only a schema that - // states something is. + // A schema that constrains nothing is the equivalent of `true`, and that is + // not only the empty object: `{ title: "Extra value" }` is annotation and + // no more. Declining on key count alone would recreate the empty form for a + // legal permissive schema, so what counts is whether an assertion is there. (typeof additional === "object" && additional !== null && - Object.keys(additional).length > 0); + Object.keys(additional).some( + (keyword) => !ANNOTATION_KEYWORDS.has(keyword), + )); const baseNames = propertiesOf(base) ?? {}; if ( restrictsAdditional && @@ -593,15 +620,31 @@ export function selectBranchIndex( // required-field gate (which accepts *any* satisfied branch) lets them be // submitted, so the form shows one shape and sends another. if (Object.keys(values).length === 0) return null; + const supplied = (name: string) => + Object.hasOwn(values, name) && values[name] !== undefined; + const satisfied = branches.filter((branch) => { const required = branch.schema.required ?? []; const own = required.filter((name) => branch.declaredFields.includes(name)); - return ( - own.length > 0 && - own.every( - (name) => Object.hasOwn(values, name) && values[name] !== undefined, - ) - ); + return own.length > 0 && own.every(supplied); }); - return satisfied.length === 1 ? branches.indexOf(satisfied[0]) : null; + if (satisfied.length === 1) return branches.indexOf(satisfied[0]); + + // Nothing is required, or several branches are satisfied. A name only ONE + // alternative declares is still evidence: supplying `phone` where only the + // SMS branch declares it names that shape as clearly as a discriminator + // would. A name more than one declares is ambiguous and says nothing. + const exclusiveTo = new Map(); + branches.forEach((branch, index) => { + for (const name of branch.declaredFields) { + exclusiveTo.set(name, exclusiveTo.has(name) ? -1 : index); + } + }); + const named = new Set( + Object.keys(values) + .filter(supplied) + .map((name) => exclusiveTo.get(name)) + .filter((index): index is number => index !== undefined && index >= 0), + ); + return named.size === 1 ? [...named][0] : null; }