fix: address Copilot review round 20 — malformed applicators, const-null under anyOf

- 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 <cliff@futurescale.com>

Author: cliffhall

clients/tui/src/utils/schemaToForm.ts

@@ -322,7 +322,9 @@ function applyConstants<T>(
 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.

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

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(
         [],

clients/web/src/utils/jsonUtils.ts

@@ -284,7 +284,10 @@ function hasMissingIn(
   schema: InspectorFormSchema,
   values: Record<string, unknown>,
 ): 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];

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)
+    );
+  });
 }
 
 /**

core/json/rootUnion.ts

@@ -100,6 +100,27 @@ export interface ResolvedRootUnion<T extends RootUnionSchema> {
   branches: RootUnionBranch<T>[];
 }
 
+/**
+ * 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<T extends RootUnionSchema>(
         : 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<T extends RootUnionSchema>(
   // 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<T>;
   for (const member of allOfMembers) {
     const branch = toBranch(member);
@@ -599,7 +619,7 @@ export function resolveRootUnion<T extends RootUnionSchema>(
     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<T extends RootUnionSchema>(
 
   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));
   });