From 4cea4e198513a739b86bcabeed0fd39675615750 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 08:31:00 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2023?= =?UTF-8?q?=20=E2=80=94=20inherited=20constants,=20branch=20null=20forms,?= =?UTF-8?q?=20test=20cast?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - matchesConstants checks own-property presence before reading a supplied value, so an absent argument legally named `constructor` no longer reads the inherited one and rules out every branch pinning that name. - anyOfAdmitsNull asks `admitsNull` about each composition-free branch instead of testing `type` alone, so a branch spelling its nullability as `{ const: null }` or `{ nullable: true }` is recognized. Refusing composed branches is what bounds the recursion. - Drop a double cast from the malformed-applicator fixture. Signed-off-by: cliffhall --- clients/web/src/test/core/jsonUtils.test.ts | 30 +++++++++++++++++++ .../web/src/test/core/nullableUnion.test.ts | 8 +++++ clients/web/src/test/core/rootUnion.test.ts | 5 +++- core/json/jsonUtils.ts | 3 ++ core/json/nullableUnion.ts | 22 +++++++------- 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 99d6dc10..f48c1234 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -181,6 +181,36 @@ describe("JSON Utils", () => { ).toEqual({ a: "x", value: 3 }); }); + it("does not read an inherited property as a supplied constant (#2123)", () => { + const pinnedOnInherited: Tool = { + name: "pinned-on-inherited", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: Object.fromEntries([ + ["constructor", { const: "a" }], + ["count", { type: "number" }], + ]), + }, + { + type: "object", + properties: Object.fromEntries([ + ["constructor", { const: "b" }], + ["other", { type: "string" }], + ]), + }, + ], + }, + }; + // `constructor` was not supplied, so it rules nothing out — `count` + // belongs to the first branch alone and settles it. + expect(convertToolParameters(pinnedOnInherited, { count: "3" })).toEqual({ + count: 3, + }); + }); + it("leaves an ambiguously typed argument uncoerced (#2123)", () => { const ambiguous: Tool = { name: "ambiguous", diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index b1ed03cd..d7090864 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -751,6 +751,14 @@ describe("admitsNull", () => { expect(admitsNull({ const: null, anyOf: [{ type: "null" }] })).toBe(true); }); + it("recognizes a branch that spells its nullability another way", () => { + // The branch admits null through `const`, not through `type`. + expect(admitsNull({ const: null, anyOf: [{ const: null }] })).toBe(true); + expect(admitsNull({ const: null, anyOf: [{ nullable: true }] })).toBe( + true, + ); + }); + it("does not override a sibling that rejects null", () => { // `const` is conjunctive with its siblings, not an override: both of // these reject every value, so claiming nullability would let the diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index f498bb99..fc795624 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -668,10 +668,13 @@ describe("resolveRootUnion", () => { // These schemas describe the wire, and every member arrives as `unknown` // — reading `anyOf: {}` as a list would throw and take all three clients // down rather than declining one malformed tool. + // A single assertion, from an `unknown`-typed value: the point is a wire + // shape TypeScript would never produce, not a cast chain. + const malformed: unknown = {}; const { base, branches } = resolveRootUnion({ type: "object", properties: { a: { type: "string" } }, - anyOf: {} as unknown as unknown[], + anyOf: malformed as unknown[], }); expect(branches).toEqual([]); expect(Object.keys(base.properties ?? {})).toEqual(["a"]); diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index de33de60..aa5030c6 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -240,6 +240,9 @@ function matchesConstants( if (typeof schema !== "object" || schema === null) return true; const constValue = (schema as { const?: unknown }).const; if (constValue === undefined) return true; + // `hasOwn`: an absent argument legally named `constructor` would otherwise + // read the inherited one and rule out every branch that pins that name. + if (!Object.hasOwn(params, name)) return true; const supplied = params[name]; return supplied === undefined || supplied === String(constValue); }); diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 518c0e86..e935b77c 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -433,17 +433,19 @@ function anyOfAdmitsNull(schema: NullableUnionSchema): boolean { if (!Array.isArray(branches)) return false; return branches.some((entry) => { const branch = toBranch(entry); - if (branch === null || !typeNamesNull(branch.type)) { - return false; - } - // 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. + if (branch === null) return false; const branchSchema = branch as NullableUnionSchema; - return ( - !nullExcludedBySiblings(branchSchema) && - !hasUnevaluatedComposition(branchSchema) - ); + // A nested union or applicator inside a branch is as opaque here as one on + // the wrapper, and refusing it also bounds this recursion: what remains is + // a composition-free schema, which `admitsNull` answers without reaching + // back into this function. + if (hasUnevaluatedComposition(branchSchema)) return false; + // Asked through `admitsNull` rather than by testing `type` alone, so a + // branch spelling its nullability another way — `{ const: null }`, + // `{ nullable: true }` — is recognized the same way the wrapper's own + // forms are, and an unsatisfiable `{ type: "null", const: "x" }` is still + // rejected by the sibling check inside it. + return admitsNull(branchSchema); }); }