From df1e39e62381ba8ef10b6d2586a03970f4e49018 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 05:21:21 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2013?= =?UTF-8?q?=20=E2=80=94=20cross-keyword=20contradictions,=20own-property?= =?UTF-8?q?=20required=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - rootUnion: keywords the two declarations state SEPARATELY can contradict too. A merged `const` is now checked against its own `type` and `enum`, so root `x: { type: "string" }` under branch `x: { const: 1 }` declines instead of seeding an immutable 1 into a string field. An integer const still satisfies a `number` type — the one direction JSON Schema widens. - TUI missingRequiredFields tests `Object.hasOwn` first: an argument legally named `constructor` resolved to the inherited one and read as supplied, and the call went out without it. Signed-off-by: cliffhall --- clients/tui/__tests__/schemaToForm.test.ts | 11 ++++ clients/tui/src/utils/schemaToForm.ts | 4 ++ clients/web/src/test/core/rootUnion.test.ts | 40 +++++++++++++++ core/json/rootUnion.ts | 56 ++++++++++++++++++--- 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index feab3486..cb9f1df2 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -808,6 +808,17 @@ describe("schemaToForm", () => { ).toEqual([]); }); + it("does not mistake an inherited property for a supplied argument", () => { + // `constructor` is a legal argument name; reading it off the prototype + // would report it as present and send the call without it. + const schema = { + type: "object", + properties: { constructor: { type: "string" } }, + required: ["constructor"], + }; + expect(missingRequiredFields(schema, {})).toEqual(["constructor"]); + }); + it("checks the root's own required fields when there is no union", () => { const schema = { type: "object", diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 0a519d94..196b7357 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -462,6 +462,10 @@ export function missingRequiredFields( ? base : branches[selectedBranchIndex(base, branches, rawValues)]!.schema; return (effective.required ?? []).filter((name) => { + // `hasOwn` first: an argument legally named `constructor` would otherwise + // resolve to the inherited one and read as supplied, and the call would go + // out without it. + if (!Object.hasOwn(decoded, name)) return true; const value = decoded[name]; return value === undefined || value === ""; }); diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index f98d8903..384703ee 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -358,6 +358,46 @@ describe("resolveRootUnion", () => { expect(branches).toHaveLength(2); }); + it("declines a branch whose const its root type rejects", () => { + // The two share no keyword, yet nothing satisfies both — and the merged + // declaration would seed an immutable `1` into a string field. + const { branches } = resolveRootUnion({ + type: "object", + properties: { x: { type: "string" } }, + anyOf: [ + { type: "object", properties: { x: { const: 1 } } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toEqual([]); + }); + + it("declines a branch whose const its root enum excludes", () => { + const { branches } = resolveRootUnion({ + type: "object", + properties: { x: { enum: ["a", "b"] } }, + anyOf: [ + { type: "object", properties: { x: { const: "c" } } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toEqual([]); + }); + + it("accepts a const its root type and enum admit", () => { + const { branches } = resolveRootUnion({ + type: "object", + properties: { x: { type: "number", enum: [1, 2] } }, + anyOf: [ + // An integer const satisfies a `number` type — the one direction JSON + // Schema widens. + { type: "object", properties: { x: { const: 1 } } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toHaveLength(2); + }); + it("declines a union whose branch contradicts the root's type for a field", () => { // `string` under a base `number` describes a value that cannot exist, so // flattening it would render one type and accept what the schema rejects. diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 04f6a561..fbb8d92e 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -224,12 +224,56 @@ function conflicts(baseProperty: unknown, branchProperty: unknown): boolean { } const left = a as Record; const right = b as Record; - return Object.keys(right).some( - (keyword) => - !ANNOTATION_KEYWORDS.has(keyword) && - keyword in left && - !sameValue(left[keyword], right[keyword]), - ); + if ( + Object.keys(right).some( + (keyword) => + !ANNOTATION_KEYWORDS.has(keyword) && + keyword in left && + !sameValue(left[keyword], right[keyword]), + ) + ) { + return true; + } + // Keywords the two sides state *separately* can still contradict each other: + // root `{ type: "string" }` under branch `{ const: 1 }` shares no keyword at + // all, yet nothing satisfies both — and the merged declaration would seed an + // immutable `1` into a field the schema rejects. + return !constSatisfiesSiblings({ ...left, ...right }); +} + +/** The JSON type name of a value, in JSON Schema's vocabulary. */ +function jsonTypeOf(value: unknown): string { + if (value === null) return "null"; + if (Array.isArray(value)) return "array"; + if (typeof value === "number") { + return Number.isInteger(value) ? "integer" : "number"; + } + return typeof value; +} + +/** + * Whether a merged declaration's `const` is one its own `type` and `enum` admit. + * + * `const` names the single value the schema accepts, so a sibling that excludes + * it leaves nothing satisfiable. An `integer` const satisfies a `number` type, + * which is the one direction JSON Schema widens. + */ +function constSatisfiesSiblings(schema: Record): boolean { + if (!("const" in schema)) return true; + const value = schema.const; + const { type, enum: allowed } = schema; + const actual = jsonTypeOf(value); + const admits = (name: unknown) => + name === actual || (name === "number" && actual === "integer"); + if (typeof type === "string" && !admits(type)) return false; + if (Array.isArray(type) && !type.some(admits)) return false; + if ( + Array.isArray(allowed) && + !allowed.some((member) => sameValue(member, value)) + ) { + return false; + } + return true; } /**