fix: address Copilot review round 16 — tie-breaking, const-aware gating, malformed declarations

- selectBranchIndex no longer stops when several branches share the supplied
  constant. A branch is dropped only when a supplied value CONTRADICTS one of
  its constants; the required-field and exclusive-name passes then run over the
  remaining candidates, so { version: 1, phone: "555" } resolves to the SMS
  branch instead of falling back to the first.
- hasMissingRequiredFields counts only the branches the values could be making
  (new branchAcceptsValues): { kind: "sms", address: "x" } supplies
  everything the EMAIL branch requires while contradicting its discriminator,
  and would otherwise enable a submit the server refuses.
- convertToolParameters ignores a malformed property declaration when branches
  agree on a type, rather than storing a non-schema as the coercion schema.

Signed-off-by: cliffhall <cliff@futurescale.com>

clients/web/src/test/core/jsonUtils.test.ts

@@ -269,6 +269,27 @@ describe("JSON Utils", () => {
       });
     });
 
+    it("ignores a malformed branch declaration when agreeing on a type (#2123)", () => {
+      const malformedDeclaration: Tool = {
+        name: "malformed-declaration",
+        inputSchema: {
+          type: "object",
+          anyOf: [
+            { type: "object", properties: { count: null as unknown, a: {} } },
+            {
+              type: "object",
+              properties: { count: { type: "number" }, b: {} },
+            },
+          ],
+        },
+      };
+      // The `null` is not a vote about the type, and must not end up standing
+      // in for one — the surviving declaration is what coerces.
+      expect(
+        convertToolParameters(malformedDeclaration, { count: "3" }),
+      ).toEqual({ count: 3 });
+    });
+
     it("coerces a value whose schema lives on a root allOf branch (#2123)", () => {
       const allOfTool: Tool = {
         name: "allof-tool",

clients/web/src/test/core/rootUnion.test.ts

@@ -811,6 +811,30 @@ describe("resolveRootUnion", () => {
       expect(selectBranchIndex(pinnedOnInherited, { x: "supplied" })).toBe(0);
     });
 
+    it("keeps looking when several branches share the supplied constant", () => {
+      // Both pin `version`, so that constant settles nothing — but `phone`
+      // belongs to one branch alone and does.
+      const versioned = resolveRootUnion({
+        type: "object",
+        anyOf: [
+          {
+            type: "object",
+            properties: {
+              version: { const: 1 },
+              address: { type: "string" },
+            },
+          },
+          {
+            type: "object",
+            properties: { version: { const: 1 }, phone: { type: "string" } },
+          },
+        ],
+      }).branches;
+      expect(selectBranchIndex(versioned, { version: 1, phone: "555" })).toBe(
+        1,
+      );
+    });
+
     it("reports none when the values identify nothing", () => {
       expect(selectBranchIndex(branches, {})).toBeNull();
       expect(selectBranchIndex(branches, { kind: "other" })).toBeNull();

clients/web/src/utils/jsonUtils.test.ts

@@ -493,6 +493,15 @@ describe("root composition (#2123)", () => {
     expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true);
   });
 
+  it("does not count a branch whose discriminator the values contradict", () => {
+    // `{ kind: "sms", address: … }` supplies everything the EMAIL branch
+    // requires while carrying a `kind` that branch rejects — and the SMS
+    // branch it does name is still missing `phone`.
+    expect(
+      hasMissingRequiredFields(UNION, { kind: "sms", address: "a@b.c" }),
+    ).toBe(true);
+  });
+
   it("allows submission once one branch is satisfied", () => {
     expect(hasMissingRequiredFields(UNION, { kind: "sms", phone: "555" })).toBe(
       false,

clients/web/src/utils/jsonUtils.ts

@@ -3,6 +3,7 @@ import {
   normalizeNullableUnion,
 } from "@inspector/core/json/nullableUnion.js";
 import {
+  branchAcceptsValues,
   resolveRootUnion,
   selectBranchIndex,
 } from "@inspector/core/json/rootUnion.js";
@@ -266,7 +267,14 @@ export function hasMissingRequiredFields(
   // direction that matters: it never blocks arguments the schema accepts.
   const { base, branches } = resolveRootUnion(schema);
   if (branches.length > 0) {
-    return branches.every((branch) => hasMissingIn(branch.schema, values));
+    // Only the branches the values could actually be making are considered: a
+    // required-name count alone would let `{ kind: "sms", address: "x" }` look
+    // like a complete *email* call, whose discriminator it contradicts.
+    const applicable = branches.filter((branch) =>
+      branchAcceptsValues(branch, values),
+    );
+    if (applicable.length === 0) return true;
+    return applicable.every((branch) => hasMissingIn(branch.schema, values));
   }
   return hasMissingIn(base, values);
 }

core/json/jsonUtils.ts

@@ -178,9 +178,13 @@ function coercionProperties(
     // specialization of a root property carries the root's keywords too.
     const declarations = branches
       .filter((branch) => branch.declaredFields.includes(name))
-      .map((branch) => branch.schema.properties?.[name]);
+      .map((branch) => branch.schema.properties?.[name])
+      // A malformed declaration (`properties: { x: null }`) is not a vote about
+      // the type, and storing it as the coercion schema would put a value that
+      // is not a schema where one is expected.
+      .filter((schema) => typeof schema === "object" && schema !== null);
     const types = new Set(declarations.map((schema) => typeNameOf(schema)));
-    if (types.size === 1) {
+    if (types.size === 1 && declarations.length > 0) {
       Object.defineProperty(properties, name, {
         value: declarations[0],
         writable: true,

core/json/rootUnion.ts

@@ -676,7 +676,18 @@ export function selectBranchIndex<T extends RootUnionSchema>(
   branches: RootUnionBranch<T>[],
   values: Record<string, unknown>,
 ): number | null {
-  const matches: number[] = [];
+  const supplied = (name: string) =>
+    // `hasOwn`: a field legally named `constructor` would otherwise read the
+    // inherited one as a supplied value.
+    Object.hasOwn(values, name) && values[name] !== undefined;
+
+  // A branch is out of the running as soon as a supplied value disagrees with
+  // one of its constants; one whose constants agree is a candidate. A constant
+  // the caller did not supply is one this identification exists to *seed*, so
+  // it is not evidence either way — which is why a branch that pins nothing
+  // relevant stays a candidate rather than being ruled in or out.
+  const candidates: number[] = [];
+  const agreeing: number[] = [];
   branches.forEach((branch, index) => {
     const pinned = Object.entries(propertiesOf(branch.schema) ?? {})
       .map(
@@ -686,58 +697,45 @@ export function selectBranchIndex<T extends RootUnionSchema>(
             (toBranch(schema) as { const?: unknown } | null)?.const,
           ] as const,
       )
-      .filter(([, constValue]) => constValue !== undefined);
+      .filter(
-    // Only the pinned names the caller actually supplied are evidence. A
+        ([name, constValue]) => constValue !== undefined && supplied(name),
-    // constant it did not supply is one this identification exists to *seed* —
+      );
-    // requiring it would mean a deep link naming `kind` alone matched no branch
+    // Structural, not reference: a `const` may be an object or an array, and
-    // whenever the branches also pin, say, a `version`.
+    // deep-link arguments arrive as freshly parsed instances that could never
-    // `hasOwn`: a pinned field legally named `constructor` would otherwise read
+    // be `===` the schema's own.
-    // the inherited one as a supplied value and rule its own branch out.
+    const agrees = pinned.every(([name, constValue]) =>
-    const supplied = pinned.filter(
+      sameValue(values[name], constValue),
-      ([name]) => Object.hasOwn(values, name) && values[name] !== undefined,
     );
-    if (
+    if (!agrees) return;
-      supplied.length > 0 &&
+    candidates.push(index);
-      // Structural, not reference: a `const` may be an object or an array, and
+    if (pinned.length > 0) agreeing.push(index);
-      // deep-link arguments arrive as freshly parsed instances that could never
-      // be `===` the schema's own.
-      supplied.every(([name, constValue]) =>
-        sameValue(values[name], constValue),
-      )
-    ) {
-      matches.push(index);
-    }
   });
-  if (matches.length === 1) return matches[0];
-  if (matches.length > 1) return null;
 
-  // No discriminator settled it. A union need not have one, and values still
+  // One branch's discriminator matched and no other's did — the plain case.
-  // belong to a shape — so fall back to the branch whose own required fields
+  if (agreeing.length === 1) return agreeing[0];
-  // the values supply, when exactly one branch's do. Without this, values for
+  if (candidates.length === 1) return candidates[0];
-  // an undiscriminated branch open the picker on the first branch while the
+  if (candidates.length === 0 || Object.keys(values).length === 0) return null;
-  // required-field gate (which accepts *any* satisfied branch) lets them be
-  // submitted, so the form shows one shape and sends another.
-  if (Object.keys(values).length === 0) return null;
-  const supplied = (name: string) =>
-    Object.hasOwn(values, name) && values[name] !== undefined;
 
-  const satisfied = branches.filter((branch) => {
+  // Several branches remain — they share the constant that was supplied, or
+  // none was. The values still belong to a shape, so keep looking among the
+  // candidates: first the branch whose own required fields they supply.
+  const satisfied = candidates.filter((index) => {
+    const branch = branches[index]!;
     const required = branch.schema.required ?? [];
     const own = required.filter((name) => branch.declaredFields.includes(name));
     return own.length > 0 && own.every(supplied);
   });
-  if (satisfied.length === 1) return branches.indexOf(satisfied[0]);
+  if (satisfied.length === 1) return satisfied[0];
 
-  // Nothing is required, or several branches are satisfied. A name only ONE
+  // Then a name only ONE candidate declares: supplying `phone` where only the
-  // alternative declares is still evidence: supplying `phone` where only the
   // SMS branch declares it names that shape as clearly as a discriminator
   // would. A name more than one declares is ambiguous and says nothing.
   const exclusiveTo = new Map<string, number>();
-  branches.forEach((branch, index) => {
+  for (const index of candidates) {
-    for (const name of branch.declaredFields) {
+    for (const name of branches[index]!.declaredFields) {
       exclusiveTo.set(name, exclusiveTo.has(name) ? -1 : index);
     }
-  });
+  }
   const named = new Set(
     Object.keys(values)
       .filter(supplied)
@@ -746,3 +744,29 @@ export function selectBranchIndex<T extends RootUnionSchema>(
   );
   return named.size === 1 ? [...named][0] : null;
 }
+
+/**
+ * Whether a branch's own constants are compatible with the values in hand — the
+ * question "could this call be making this shape".
+ *
+ * A required-field check alone is not that question: in an email/SMS union,
+ * `{ kind: "sms", address: "x" }` supplies everything the *email* branch
+ * requires while carrying a discriminator that branch rejects, so treating it
+ * as satisfiable would enable a submit the server refuses.
+ */
+export function branchAcceptsValues<T extends RootUnionSchema>(
+  branch: RootUnionBranch<T>,
+  values: Record<string, unknown>,
+): 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);
+    },
+  );
+}