From d334710d6f89b74ceca2092e83a5aa6c04aa1f36 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 07:26:49 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2020?= =?UTF-8?q?=20=E2=80=94=20malformed=20applicators,=20const-null=20under=20?= =?UTF-8?q?anyOf?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - rootUnion reads every composition keyword through `membersOf` and every `required` through `requiredOf`. These schemas describe the wire and each member arrives as `unknown`, so `anyOf: {}` or `required: "x"` really can arrive — and reading them as lists threw, taking all three clients down rather than declining one malformed tool. Both web and TUI required-field checks are guarded the same way. - admitsNull evaluates a sibling `anyOf` beside a `const: null` instead of refusing outright: the union decides in both directions, and `{ const: null, anyOf: [{ type: "null" }] }` admits null as plainly as an all-string union rejects it. Refusing it left a read-only required field seeded with the only value it accepts and submit permanently disabled. Signed-off-by: cliffhall --- clients/tui/src/utils/schemaToForm.ts | 7 ++- .../web/src/test/core/nullableUnion.test.ts | 6 +++ clients/web/src/test/core/rootUnion.test.ts | 29 +++++++++++ clients/web/src/utils/jsonUtils.ts | 5 +- core/json/nullableUnion.ts | 51 ++++++++++--------- core/json/rootUnion.ts | 40 +++++++++++---- 6 files changed, 102 insertions(+), 36 deletions(-) diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 178a9084..dd034cf6 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -322,7 +322,9 @@ function applyConstants( function buildFields(schema: JsonSchemaObject): FormField[] { const fields: FormField[] = []; const properties = schema.properties || {}; - const required = schema.required || []; + // `Array.isArray`, not `|| []`: a nonconforming server can send + // `required: "x"`, and `.includes` on a string silently matches substrings. + const required = Array.isArray(schema.required) ? schema.required : []; for (const [key, prop] of Object.entries(properties)) { // `properties` values are `unknown` (the SDK schema admits anything), so @@ -463,7 +465,8 @@ export function missingRequiredFields( ? base : branches[selectedBranchIndex(base, branches, rawValues)]!.schema; const properties = effective.properties ?? {}; - return (effective.required ?? []).filter((name) => { + const required = Array.isArray(effective.required) ? effective.required : []; + return 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. diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index cbe0ea34..b1ed03cd 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -745,6 +745,12 @@ describe("admitsNull", () => { ); }); + it("admits null when a sibling anyOf branch does", () => { + // The union decides in both directions: an all-string one rejects null, + // and a null branch admits it just as plainly. + expect(admitsNull({ const: null, anyOf: [{ type: "null" }] })).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 85ff01cb..f498bb99 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -664,6 +664,35 @@ describe("resolveRootUnion", () => { expect(branches).toEqual([]); }); + it("survives a composition keyword that is not a list", () => { + // 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. + const { base, branches } = resolveRootUnion({ + type: "object", + properties: { a: { type: "string" } }, + anyOf: {} as unknown as unknown[], + }); + expect(branches).toEqual([]); + expect(Object.keys(base.properties ?? {})).toEqual(["a"]); + }); + + it("survives a required that is not a list", () => { + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { a: { type: "string" } }, + required: "a", + }, + { type: "object", properties: { b: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toHaveLength(2); + expect(branches[0].schema.required).toBeUndefined(); + }); + it("declines an empty union", () => { expect(resolveRootUnion({ type: "object", anyOf: [] }).branches).toEqual( [], diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 85d0d00c..4f1838c8 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -284,7 +284,10 @@ function hasMissingIn( schema: InspectorFormSchema, values: Record, ): boolean { - const required = schema.required ?? []; + // `Array.isArray`, not `?? []`: these schemas describe the wire, and a + // nonconforming server can send `required: "x"` — which `.some` would throw + // on, taking the whole panel down rather than one malformed tool. + const required = Array.isArray(schema.required) ? schema.required : []; const properties = schema.properties ?? {}; return required.some((field) => { const value = values[field]; diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 1709bd67..518c0e86 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -397,10 +397,13 @@ export function admitsNull(schema: NullableUnionSchema): boolean { // would let the gate accept a `null` the schema forbids. The opaque // applicators (`not`, `allOf`, `oneOf`) are already refused above. if (schema.const === null) { - return ( - schema.anyOf === undefined && - (schema.nullable === true || typeAdmitsNull(schema.type)) - ); + if (schema.nullable !== true && !typeAdmitsNull(schema.type)) return false; + // A sibling `anyOf` is conjunctive with the `const`, so it decides too — + // but it decides in *both* directions: `{ const: null, anyOf: [{ type: + // "null" }] }` admits null just as plainly as an all-string union rejects + // it. Evaluating the branches is what keeps the first case usable, since + // the field is rendered read-only and seeded with the only value it takes. + return schema.anyOf === undefined || anyOfAdmitsNull(schema); } if (schema.nullable === true) { @@ -421,25 +424,27 @@ export function admitsNull(schema: NullableUnionSchema): boolean { } return typeNamesNull(schema.type); } - return ( - schema.anyOf?.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 its own applicators are as opaque - // here as the wrapper's. - const branchSchema = branch as NullableUnionSchema; - // 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. - return ( - !nullExcludedBySiblings(branchSchema) && - !hasUnevaluatedComposition(branchSchema) - ); - }) ?? false - ); + return anyOfAdmitsNull(schema); +} + +/** Whether some `anyOf` branch is one that admits `null`. */ +function anyOfAdmitsNull(schema: NullableUnionSchema): boolean { + const branches = schema.anyOf; + 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. + const branchSchema = branch as NullableUnionSchema; + return ( + !nullExcludedBySiblings(branchSchema) && + !hasUnevaluatedComposition(branchSchema) + ); + }); } /** diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 5c77bd28..74894efd 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -100,6 +100,27 @@ export interface ResolvedRootUnion { branches: RootUnionBranch[]; } +/** + * A composition keyword's members, or `[]` when the value is not a list. + * + * These annotations describe the wire, and the wire is whatever a server sent: + * the web client narrows its schema with a cast and every member arrives as + * `unknown`, so `anyOf: {}` really can reach this module. Reading it as a list + * would throw and take all three clients down with it, which is a worse answer + * than declining to flatten a schema nobody can interpret. + */ +function membersOf(value: readonly unknown[] | undefined): readonly unknown[] { + return Array.isArray(value) ? value : []; +} + +/** A schema's `required`, keeping only the string entries a list-shaped one holds. */ +function requiredOf(schema: RootUnionSchema): string[] { + const { required } = schema; + return Array.isArray(required) + ? required.filter((name): name is string => typeof name === "string") + : []; +} + /** Narrow a composition member to a readable object, or `null` if it isn't one. */ function toBranch(value: unknown): RootUnionSchema | null { if (typeof value !== "object" || value === null || Array.isArray(value)) { @@ -441,9 +462,9 @@ export function declaresAnyFields( // tool with `{}` on the strength of something never read. if (schema.$ref !== undefined) return true; const members = [ - ...(schema.allOf ?? []), - ...(schema.anyOf ?? []), - ...(schema.oneOf ?? []), + ...membersOf(schema.allOf), + ...membersOf(schema.anyOf), + ...membersOf(schema.oneOf), ]; return members.some((member) => { const branch = toBranch(member); @@ -479,11 +500,10 @@ function mergeBranch( : branchProperty, ]), ]); + const baseRequired = requiredOf(base); const required = [ - ...(base.required ?? []), - ...(branch.required ?? []).filter( - (name) => !(base.required ?? []).includes(name), - ), + ...baseRequired, + ...requiredOf(branch).filter((name) => !baseRequired.includes(name)), ]; // One cast, owned here: a branch member is `unknown` on the wire, so its // property schemas are whatever the server sent however `T` declares them — @@ -576,7 +596,7 @@ export function resolveRootUnion( // composition keywords stay on it, and no union is offered either, since a // branch would otherwise be merged against a base whose constraints are not // all known. - const allOfMembers = schema.allOf ?? []; + const allOfMembers = membersOf(schema.allOf); let merged = schema as ResolvedSchema; for (const member of allOfMembers) { const branch = toBranch(member); @@ -599,7 +619,7 @@ export function resolveRootUnion( return { base, branches: [] }; } const isExclusiveUnion = schema.oneOf !== undefined; - const members = schema.oneOf ?? schema.anyOf ?? []; + const members = membersOf(schema.oneOf ?? schema.anyOf); const branches = members.map(toBranch); if ( branches.length === 0 || @@ -745,7 +765,7 @@ export function narrowBySuppliedNames( const satisfied = candidates.filter((index) => { const branch = branches[index]!; - const required = branch.schema.required ?? []; + const required = requiredOf(branch.schema); const own = required.filter((name) => branch.declaredFields.includes(name)); return own.length > 0 && own.every((name) => supplied.has(name)); });