From 7792ad71e9da7b5d6ec6f03a522f741d529d8cb3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 06:23:54 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2016?= =?UTF-8?q?=20=E2=80=94=20tie-breaking,=20const-aware=20gating,=20malforme?= =?UTF-8?q?d=20declarations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - selectBranchIndex no longer stops when several branches share the supplied constant. A branch is dropped only when a supplied value CONTRADICTS one of its constants; the required-field and exclusive-name passes then run over the remaining candidates, so { version: 1, phone: "555" } resolves to the SMS branch instead of falling back to the first. - hasMissingRequiredFields counts only the branches the values could be making (new branchAcceptsValues): { kind: "sms", address: "x" } supplies everything the EMAIL branch requires while contradicting its discriminator, and would otherwise enable a submit the server refuses. - convertToolParameters ignores a malformed property declaration when branches agree on a type, rather than storing a non-schema as the coercion schema. Signed-off-by: cliffhall --- clients/web/src/test/core/jsonUtils.test.ts | 21 ++++ clients/web/src/test/core/rootUnion.test.ts | 24 +++++ clients/web/src/utils/jsonUtils.test.ts | 9 ++ clients/web/src/utils/jsonUtils.ts | 10 +- core/json/jsonUtils.ts | 8 +- core/json/rootUnion.ts | 102 ++++++++++++-------- 6 files changed, 132 insertions(+), 42 deletions(-) diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index fea4a725..c34a42cf 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -269,6 +269,27 @@ describe("JSON Utils", () => { }); }); + it("ignores a malformed branch declaration when agreeing on a type (#2123)", () => { + const malformedDeclaration: Tool = { + name: "malformed-declaration", + inputSchema: { + type: "object", + anyOf: [ + { type: "object", properties: { count: null as unknown, a: {} } }, + { + type: "object", + properties: { count: { type: "number" }, b: {} }, + }, + ], + }, + }; + // The `null` is not a vote about the type, and must not end up standing + // in for one — the surviving declaration is what coerces. + expect( + convertToolParameters(malformedDeclaration, { count: "3" }), + ).toEqual({ count: 3 }); + }); + it("coerces a value whose schema lives on a root allOf branch (#2123)", () => { const allOfTool: Tool = { name: "allof-tool", diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index b46183e4..85ff01cb 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -811,6 +811,30 @@ describe("resolveRootUnion", () => { expect(selectBranchIndex(pinnedOnInherited, { x: "supplied" })).toBe(0); }); + it("keeps looking when several branches share the supplied constant", () => { + // Both pin `version`, so that constant settles nothing — but `phone` + // belongs to one branch alone and does. + const versioned = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { + version: { const: 1 }, + address: { type: "string" }, + }, + }, + { + type: "object", + properties: { version: { const: 1 }, phone: { type: "string" } }, + }, + ], + }).branches; + expect(selectBranchIndex(versioned, { version: 1, phone: "555" })).toBe( + 1, + ); + }); + it("reports none when the values identify nothing", () => { expect(selectBranchIndex(branches, {})).toBeNull(); expect(selectBranchIndex(branches, { kind: "other" })).toBeNull(); diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 1d2fd8ae..82614c34 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -493,6 +493,15 @@ describe("root composition (#2123)", () => { expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true); }); + it("does not count a branch whose discriminator the values contradict", () => { + // `{ kind: "sms", address: … }` supplies everything the EMAIL branch + // requires while carrying a `kind` that branch rejects — and the SMS + // branch it does name is still missing `phone`. + expect( + hasMissingRequiredFields(UNION, { kind: "sms", address: "a@b.c" }), + ).toBe(true); + }); + it("allows submission once one branch is satisfied", () => { expect(hasMissingRequiredFields(UNION, { kind: "sms", phone: "555" })).toBe( false, diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 823e4f33..a93f330c 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -3,6 +3,7 @@ import { normalizeNullableUnion, } from "@inspector/core/json/nullableUnion.js"; import { + branchAcceptsValues, resolveRootUnion, selectBranchIndex, } from "@inspector/core/json/rootUnion.js"; @@ -266,7 +267,14 @@ export function hasMissingRequiredFields( // direction that matters: it never blocks arguments the schema accepts. const { base, branches } = resolveRootUnion(schema); if (branches.length > 0) { - return branches.every((branch) => hasMissingIn(branch.schema, values)); + // Only the branches the values could actually be making are considered: a + // required-name count alone would let `{ kind: "sms", address: "x" }` look + // like a complete *email* call, whose discriminator it contradicts. + const applicable = branches.filter((branch) => + branchAcceptsValues(branch, values), + ); + if (applicable.length === 0) return true; + return applicable.every((branch) => hasMissingIn(branch.schema, values)); } return hasMissingIn(base, values); } diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index 32d53166..e8b5b9c4 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -178,9 +178,13 @@ function coercionProperties( // specialization of a root property carries the root's keywords too. const declarations = branches .filter((branch) => branch.declaredFields.includes(name)) - .map((branch) => branch.schema.properties?.[name]); + .map((branch) => branch.schema.properties?.[name]) + // A malformed declaration (`properties: { x: null }`) is not a vote about + // the type, and storing it as the coercion schema would put a value that + // is not a schema where one is expected. + .filter((schema) => typeof schema === "object" && schema !== null); const types = new Set(declarations.map((schema) => typeNameOf(schema))); - if (types.size === 1) { + if (types.size === 1 && declarations.length > 0) { Object.defineProperty(properties, name, { value: declarations[0], writable: true, diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index a9c31680..92247a33 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -676,7 +676,18 @@ export function selectBranchIndex( branches: RootUnionBranch[], values: Record, ): number | null { - const matches: number[] = []; + const supplied = (name: string) => + // `hasOwn`: a field legally named `constructor` would otherwise read the + // inherited one as a supplied value. + Object.hasOwn(values, name) && values[name] !== undefined; + + // A branch is out of the running as soon as a supplied value disagrees with + // one of its constants; one whose constants agree is a candidate. A constant + // the caller did not supply is one this identification exists to *seed*, so + // it is not evidence either way — which is why a branch that pins nothing + // relevant stays a candidate rather than being ruled in or out. + const candidates: number[] = []; + const agreeing: number[] = []; branches.forEach((branch, index) => { const pinned = Object.entries(propertiesOf(branch.schema) ?? {}) .map( @@ -686,58 +697,45 @@ export function selectBranchIndex( (toBranch(schema) as { const?: unknown } | null)?.const, ] as const, ) - .filter(([, constValue]) => constValue !== undefined); - // Only the pinned names the caller actually supplied are evidence. A - // constant it did not supply is one this identification exists to *seed* — - // requiring it would mean a deep link naming `kind` alone matched no branch - // whenever the branches also pin, say, a `version`. - // `hasOwn`: a pinned field legally named `constructor` would otherwise read - // the inherited one as a supplied value and rule its own branch out. - const supplied = pinned.filter( - ([name]) => Object.hasOwn(values, name) && values[name] !== undefined, + .filter( + ([name, constValue]) => constValue !== undefined && supplied(name), + ); + // Structural, not reference: a `const` may be an object or an array, and + // deep-link arguments arrive as freshly parsed instances that could never + // be `===` the schema's own. + const agrees = pinned.every(([name, constValue]) => + sameValue(values[name], constValue), ); - if ( - supplied.length > 0 && - // Structural, not reference: a `const` may be an object or an array, and - // deep-link arguments arrive as freshly parsed instances that could never - // be `===` the schema's own. - supplied.every(([name, constValue]) => - sameValue(values[name], constValue), - ) - ) { - matches.push(index); - } + if (!agrees) return; + candidates.push(index); + if (pinned.length > 0) agreeing.push(index); }); - if (matches.length === 1) return matches[0]; - if (matches.length > 1) return null; - // No discriminator settled it. A union need not have one, and values still - // belong to a shape — so fall back to the branch whose own required fields - // the values supply, when exactly one branch's do. Without this, values for - // an undiscriminated branch open the picker on the first branch while the - // 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; + // One branch's discriminator matched and no other's did — the plain case. + if (agreeing.length === 1) return agreeing[0]; + if (candidates.length === 1) return candidates[0]; + if (candidates.length === 0 || Object.keys(values).length === 0) return null; - const satisfied = branches.filter((branch) => { + // Several branches remain — they share the constant that was supplied, or + // none was. The values still belong to a shape, so keep looking among the + // candidates: first the branch whose own required fields they supply. + const satisfied = candidates.filter((index) => { + const branch = branches[index]!; const required = branch.schema.required ?? []; const own = required.filter((name) => branch.declaredFields.includes(name)); return own.length > 0 && own.every(supplied); }); - if (satisfied.length === 1) return branches.indexOf(satisfied[0]); + if (satisfied.length === 1) return satisfied[0]; - // Nothing is required, or several branches are satisfied. A name only ONE - // alternative declares is still evidence: supplying `phone` where only the + // Then a name only ONE candidate declares: 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) { + for (const index of candidates) { + for (const name of branches[index]!.declaredFields) { exclusiveTo.set(name, exclusiveTo.has(name) ? -1 : index); } - }); + } const named = new Set( Object.keys(values) .filter(supplied) @@ -746,3 +744,29 @@ export function selectBranchIndex( ); return named.size === 1 ? [...named][0] : null; } + +/** + * Whether a branch's own constants are compatible with the values in hand — the + * question "could this call be making this shape". + * + * A required-field check alone is not that question: in an email/SMS union, + * `{ kind: "sms", address: "x" }` supplies everything the *email* branch + * requires while carrying a discriminator that branch rejects, so treating it + * as satisfiable would enable a submit the server refuses. + */ +export function branchAcceptsValues( + branch: RootUnionBranch, + values: Record, +): boolean { + return Object.entries(propertiesOf(branch.schema) ?? {}).every( + ([name, schema]) => { + const constValue = (toBranch(schema) as { const?: unknown } | null) + ?.const; + if (constValue === undefined) return true; + if (!Object.hasOwn(values, name) || values[name] === undefined) { + return true; + } + return sameValue(values[name], constValue); + }, + ); +}