diff --git a/README.md b/README.md index c8296ebd..8f28891f 100644 --- a/README.md +++ b/README.md @@ -313,7 +313,7 @@ What it declines to flatten is as deliberate as what it flattens, and every case - **A union whose members are not all field-carrying object schemas** — including one whose member `type` rules objects out, since tool arguments are a JSON object and such a member can never match. A picker whose options render nothing is no better than no picker. - **A branch that restates a constraint the root already states.** The two are conjunctive, so root `minimum: 10` under branch `minimum: 0` is still 10, disjoint `enum`s leave nothing satisfiable, and `type: "string"` under `type: "number"` describes a value that cannot exist — rendering either side would accept what the schema rejects. A property both declare *compatibly* is merged rather than replaced, so a root's `minimum` survives a branch's `maximum`, and a disagreement about `title`/`description` is not a conflict at all. - **A composition member stating anything the merge cannot apply.** Only `type`, `properties` and `required` are folded in, so a member carrying a nested `allOf`/`anyOf`, a `not`, an `additionalProperties`, or a `$ref` would have that constraint erased along with the keyword — turning an unsatisfiable schema (`allOf: [false, …]` admits nothing) into a fillable form. `allOf` members are checked against the accumulated merge rather than the root alone, so two of them contradicting each other is caught even when neither contradicts the root. -- **A `oneOf` whose alternatives are not mutually exclusive.** `oneOf` demands that *exactly one* alternative match, which flattening cannot preserve — the branches are offered as if any would do. It is only safe with a discriminator (a property every branch pins to a `const` of its own), so an undiscriminated `oneOf` is declined. `anyOf` makes no such claim and is offered either way. +- **A `oneOf` whose alternatives are not mutually exclusive.** `oneOf` demands that *exactly one* alternative match, which flattening cannot preserve — the branches are offered as if any would do. It is only safe with a discriminator: a property every branch pins to a `const` of its own **and requires**, since an optional one leaves `{}` matching every branch. An undiscriminated `oneOf` is declined; `anyOf` makes no such claim and is offered either way. - **A union that adds fields under a restrictive root `additionalProperties`.** That keyword constrains whatever its *sibling* `properties` does not name, so a root `additionalProperties: false` rejects every field the branches add — flattening would move them beside the keyword, where they read as allowed. An empty schema (`{}`) constrains nothing and is treated as permissive. - **A schema carrying both `oneOf` and `anyOf`** — independent keywords a value satisfies *together*, not two spellings of one union, so reading one and dropping the other omits real constraints while looking complete. Satisfying both honestly means the cross product of their alternatives, which no real schema has yet asked for. - **`not`**, which is not interpreted at all: there is no faithful form for "anything except this". diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index f48c1234..486f902b 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -135,6 +135,9 @@ describe("JSON Utils", () => { kind: { type: "string", const: "a" }, value: { type: "number" }, }, + // Required, or the alternatives are not exclusive and the + // resolver declines the `oneOf` outright. + required: ["kind"], }, { type: "object", @@ -142,6 +145,7 @@ describe("JSON Utils", () => { kind: { type: "string", const: "b" }, value: { type: "boolean" }, }, + required: ["kind"], }, ], }, diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index fc795624..c408680a 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -444,6 +444,81 @@ describe("resolveRootUnion", () => { expect(branches).toEqual([]); }); + describe("values it compares and types it recognizes", () => { + it("offers a branch whose type list names object", () => { + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: ["object", "null"], + properties: { a: { type: "string" } }, + }, + { type: "object", properties: { b: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toHaveLength(2); + }); + + it("compares array-valued constants structurally", () => { + const { branches } = resolveRootUnion({ + type: "object", + required: ["tag"], + oneOf: [ + { + type: "object", + properties: { tag: { const: [1, 2] }, x: { type: "string" } }, + }, + { + type: "object", + properties: { tag: { const: [1, 2] }, y: { type: "string" } }, + }, + ], + }); + // The same value twice, so the alternatives are not exclusive. + expect(branches).toEqual([]); + }); + + it("treats a keyword explicitly set to undefined as a disagreement", () => { + const { branches } = resolveRootUnion({ + type: "object", + properties: { x: { minimum: undefined } }, + anyOf: [ + { type: "object", properties: { x: { minimum: 1 } } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toEqual([]); + }); + + it("recognizes each JSON type when checking a const", () => { + const accepts = (type: unknown, constValue: unknown) => + resolveRootUnion({ + type: "object", + properties: { x: { type } as unknown }, + anyOf: [ + { type: "object", properties: { x: { const: constValue } } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }).branches.length > 0; + + expect(accepts("null", null)).toBe(true); + expect(accepts("array", [1])).toBe(true); + expect(accepts("number", 1)).toBe(true); + expect(accepts(["string", "integer"], 1)).toBe(true); + expect(accepts("integer", 1.5)).toBe(false); + expect(accepts(["string", "boolean"], 1)).toBe(false); + }); + + it("emits no properties when neither side declares any", () => { + const { base } = resolveRootUnion({ + type: "object", + allOf: [{ type: "object", required: ["a"] }], + }); + expect(base.properties).toBeUndefined(); + expect(base.required).toEqual(["a"]); + }); + }); + describe("branch labels", () => { it("uses the branch's own title first", () => { const { branches } = resolveRootUnion({ @@ -597,6 +672,43 @@ describe("resolveRootUnion", () => { expect(branches).toEqual([]); }); + it("declines a oneOf whose discriminator is optional", () => { + // Two branches pinning an OPTIONAL `kind` both match `{}`, so arguments + // omitting it satisfy more than one alternative. + const { branches } = resolveRootUnion({ + type: "object", + oneOf: [ + { + type: "object", + properties: { kind: { const: "a" }, x: { type: "string" } }, + }, + { + type: "object", + properties: { kind: { const: "b" }, y: { type: "string" } }, + }, + ], + }); + expect(branches).toEqual([]); + }); + + it("accepts a discriminator the root requires", () => { + const { branches } = resolveRootUnion({ + type: "object", + required: ["kind"], + oneOf: [ + { + type: "object", + properties: { kind: { const: "a" }, x: { type: "string" } }, + }, + { + type: "object", + properties: { kind: { const: "b" }, y: { type: "string" } }, + }, + ], + }); + expect(branches).toHaveLength(2); + }); + it("declines a oneOf whose named discriminator does not distinguish", () => { const { branches } = resolveRootUnion({ type: "object", diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 74894efd..ad9f31da 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -136,17 +136,27 @@ function admitsObject(schema: RootUnionSchema): boolean { return Array.isArray(type) ? type.includes("object") : type === "object"; } -/** A readable `properties` map, or `null` when the value is not one. */ -function propertiesOf(schema: RootUnionSchema): Record | null { +/** Whether a schema's `properties` is a readable map rather than junk. */ +function hasReadableProperties(schema: RootUnionSchema): boolean { const { properties } = schema; - if ( - typeof properties !== "object" || - properties === null || - Array.isArray(properties) - ) { - return null; - } - return properties; + return ( + typeof properties === "object" && + properties !== null && + !Array.isArray(properties) + ); +} + +/** + * A schema's `properties`, or an empty map when it has none — or when what it + * has is not a map at all, which a wire schema really can be. Total by design: + * every caller but {@link isOfferable} wants to enumerate whatever is there, + * and a nullable return would leave each of them carrying a `?? {}` that + * nothing can reach. + */ +function propertiesOf(schema: RootUnionSchema): Record { + return hasReadableProperties(schema) + ? (schema.properties as Record) + : {}; } /** @@ -166,10 +176,9 @@ function propertiesOf(schema: RootUnionSchema): Record | null { * as a fillable form would offer a call that cannot be valid. */ function isOfferable(branch: RootUnionSchema): boolean { - const properties = propertiesOf(branch); return ( - properties !== null && - Object.keys(properties).length > 0 && + hasReadableProperties(branch) && + Object.keys(propertiesOf(branch)).length > 0 && admitsObject(branch) ); } @@ -364,8 +373,8 @@ function conflictsWithBase( base: RootUnionSchema, branch: RootUnionSchema, ): boolean { - const baseProperties = propertiesOf(base) ?? {}; - const branchProperties = propertiesOf(branch) ?? {}; + const baseProperties = propertiesOf(base); + const branchProperties = propertiesOf(branch); return Object.entries(branchProperties).some( ([name, branchProperty]) => // `hasOwn`, not `in`: `properties` is a JSON record, so `constructor` and @@ -388,12 +397,19 @@ function conflictsWithBase( function hasDiscriminator( members: RootUnionSchema[], named: string | undefined, + rootRequired: string[], ): boolean { - const first = propertiesOf(members[0] ?? {}) ?? {}; + const first = propertiesOf(members[0] ?? {}); const candidates = named !== undefined ? [named] : Object.keys(first); return candidates.some((name) => { + // Required, or it discriminates nothing: two branches pinning an OPTIONAL + // `kind` to different constants both match `{}`, so arguments omitting it + // satisfy more than one alternative — exactly what `oneOf` forbids. const constants = members.map((member) => { - const property = toBranch((propertiesOf(member) ?? {})[name]) as { + if (!rootRequired.includes(name) && !requiredOf(member).includes(name)) { + return undefined; + } + const property = toBranch(propertiesOf(member)[name]) as { const?: unknown; } | null; return property?.const; @@ -438,8 +454,8 @@ function addsForbiddenNames( member: RootUnionSchema, ): boolean { if (!restrictsAdditional(base)) return false; - const baseNames = propertiesOf(base) ?? {}; - return Object.keys(propertiesOf(member) ?? {}).some( + const baseNames = propertiesOf(base); + return Object.keys(propertiesOf(member)).some( (name) => !Object.hasOwn(baseNames, name), ); } @@ -456,7 +472,7 @@ export function declaresAnyFields( schema: RootUnionSchema | undefined, ): boolean { if (schema === undefined) return false; - if (Object.keys(propertiesOf(schema) ?? {}).length > 0) return true; + if (Object.keys(propertiesOf(schema)).length > 0) return true; // A `$ref`'s shape is unknown rather than empty, so it counts. Reporting "no // fields" for `anyOf: [{ $ref: … }, { $ref: … }]` would auto-invoke an App // tool with `{}` on the strength of something never read. @@ -485,8 +501,8 @@ function mergeBranch( base: T, branch: RootUnionSchema, ): ResolvedSchema { - const baseProperties = propertiesOf(base) ?? {}; - const branchProperties = propertiesOf(branch) ?? {}; + const baseProperties = propertiesOf(base); + const branchProperties = propertiesOf(branch); // Built through `fromEntries` rather than by assignment: a property named // `__proto__` is a legal argument name, and assigning it would invoke the // legacy prototype setter instead of creating an own property — losing the @@ -544,7 +560,7 @@ function branchLabel( if (typeof branch.title === "string" && branch.title.trim() !== "") { return branch.title; } - const properties = propertiesOf(branch) ?? {}; + const properties = propertiesOf(branch); const constOf = (name: string): string | null => { const property = toBranch(properties[name]) as { const?: unknown } | null; const value = property?.const; @@ -662,6 +678,7 @@ export function resolveRootUnion( !hasDiscriminator( branches as RootUnionSchema[], schema.discriminator?.propertyName, + requiredOf(base), ) ) { return { base, branches: [] }; @@ -677,7 +694,7 @@ export function resolveRootUnion( // `properties`/`required` off the branch, so the merge stays that way. schema: mergeBranch(base, branch), label: branchLabel(branch, index, discriminatorProperty), - declaredFields: Object.keys(propertiesOf(branch) ?? {}), + declaredFields: Object.keys(propertiesOf(branch)), })), }; } @@ -709,7 +726,7 @@ export function selectBranchIndex( const candidates: number[] = []; const agreeing: number[] = []; branches.forEach((branch, index) => { - const pinned = Object.entries(propertiesOf(branch.schema) ?? {}) + const pinned = Object.entries(propertiesOf(branch.schema)) .map( ([name, schema]) => [ @@ -798,15 +815,12 @@ 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); - }, - ); + 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); + }); }