fix: address Copilot review round 4 — allOf faithfulness, const null, branch-aware seeding

- rootUnion: an `allOf` member is foldable only when it states nothing beyond
  what the merge applies (`type`/`properties`/`required` plus annotations),
  so a nested `anyOf`, a `not`, or an `additionalProperties` can no longer
  be erased along with the keyword; members are checked against the
  ACCUMULATED merge, so two of them contradicting each other is caught even
  when neither contradicts the root.
- admitsNull: `const: null` admits null, so a required field pinned to it is
  no longer seeded with the one value it accepts and then reported missing.
- collectSchemaDefaults takes the values a caller is about to overlay, and
  seeds the branch THEY identify — the App deep link passes its `appArgs`, so
  branch 0's defaults can no longer sit invisibly under branch 1's arguments.
- The branch inference moves into core as `selectBranchIndex`, shared by the
  seeding and the form's picker so the two cannot disagree.

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

clients/web/src/components/groups/SchemaForm/SchemaForm.tsx

@@ -31,7 +31,10 @@ import {
   isStringEnum,
   normalizeNullableUnion,
 } from "@inspector/core/json/nullableUnion.js";
-import { resolveRootUnion } from "@inspector/core/json/rootUnion.js";
+import {
+  resolveRootUnion,
+  selectBranchIndex,
+} from "@inspector/core/json/rootUnion.js";
 import { collectSchemaDefaults } from "../../../utils/jsonUtils";
 
 const FieldLabel = Text.withProps({
@@ -571,36 +574,6 @@ function resolveValue(
   return getDefaultValue(fieldSchema);
 }
 
-/**
- * The branch a set of values already identifies, or `null` when they identify
- * none uniquely.
- *
- * A discriminated union pins its discriminator with `const`, so values carrying
- * one name the branch they belong to. This matters because the form is not
- * always mounted empty: an App deep link overlays `appArgs` on the initial
- * defaults, so `{ kind: "sms", … }` can arrive while the picker would otherwise
- * open on the first branch — showing one shape's controls while a differently
- * shaped set of values sits underneath, ready to be submitted (#2123).
- */
-function matchBranchIndex(
-  branches: { schema: InspectorFormSchema }[],
-  values: Record<string, unknown>,
-): number | null {
-  const matches: number[] = [];
-  branches.forEach((branch, index) => {
-    const pinned = Object.entries(branch.schema.properties ?? {}).filter(
-      ([, fieldSchema]) => fieldSchema.const !== undefined,
-    );
-    if (
-      pinned.length > 0 &&
-      pinned.every(([name, fieldSchema]) => values[name] === fieldSchema.const)
-    ) {
-      matches.push(index);
-    }
-  });
-  return matches.length === 1 ? matches[0] : null;
-}
-
 export function SchemaForm({
   schema,
   values,
@@ -619,7 +592,7 @@ export function SchemaForm({
   // property of this rendering, not of the arguments: `values` carries what the
   // user typed, and nothing in it names a branch.
   const [branchIndex, setBranchIndex] = useState(
-    () => matchBranchIndex(branches, values) ?? 0,
+    () => selectBranchIndex(branches, values) ?? 0,
   );
   // A form reused for another entity can be handed a shorter union, so the
   // index is clamped rather than trusted — `resetKey` resets it below, but a
@@ -657,7 +630,7 @@ export function SchemaForm({
     // same reason enlargement does — reset to whichever branch the new values
     // identify, so the visible selection cannot disagree with what would be
     // submitted, and to the first when they identify none.
-    setBranchIndex(matchBranchIndex(branches, values) ?? 0);
+    setBranchIndex(selectBranchIndex(branches, values) ?? 0);
   });
 
   // Stable so a field's reporting effect subscribes once, not per render. The

clients/web/src/components/views/InspectorView/InspectorView.tsx

@@ -1035,8 +1035,16 @@ export function InspectorView({
     // value is absent from `formValues`, the schema-form's validity check
     // fails, and Open App is silently disabled — an automated driver's click
     // then no-ops and the iframe-wait spins forever.
+    // The args are passed to the seeding too, not just overlaid on it: for a
+    // schema whose arguments are a root union they can name a branch other
+    // than the first, and defaults seeded from the wrong branch would sit in
+    // the submitted arguments where the form — showing the branch the args
+    // identify — never displays them (#2123).
     const formValues = {
-      ...collectSchemaDefaults(toFormSchema(target.inputSchema) ?? {}),
+      ...collectSchemaDefaults(
+        toFormSchema(target.inputSchema) ?? {},
+        deepLink.appArgs ?? {},
+      ),
       ...deepLink.appArgs,
     };
     // Seed the selection directly rather than routing through

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

@@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
 import {
   declaresAnyFields,
   resolveRootUnion,
+  selectBranchIndex,
 } from "@inspector/core/json/rootUnion.js";
 
 const EMAIL = {
@@ -116,6 +117,36 @@ describe("resolveRootUnion", () => {
     expect(branches).toEqual([]);
   });
 
+  it("leaves an allOf member carrying a constraint the merge cannot apply", () => {
+    // Only `properties`/`required` are merged, so a member stating anything
+    // further would have that constraint erased with the keyword.
+    for (const member of [
+      { type: "object", properties: { x: {} }, additionalProperties: false },
+      { type: "object", properties: { x: {} }, not: { properties: {} } },
+      { type: "string", properties: { x: {} } },
+    ]) {
+      const { base } = resolveRootUnion({
+        type: "object" as const,
+        allOf: [member as unknown],
+      });
+      expect(base.allOf).toHaveLength(1);
+      expect(base.properties).toBeUndefined();
+    }
+  });
+
+  it("declines an allOf whose members contradict each other", () => {
+    // Neither conflicts with the root, which declares no `x` at all.
+    const { base } = resolveRootUnion({
+      type: "object",
+      allOf: [
+        { type: "object", properties: { x: { minimum: 10 } } },
+        { type: "object", properties: { x: { minimum: 0 } } },
+      ],
+    });
+    expect(base.allOf).toHaveLength(2);
+    expect(base.properties).toBeUndefined();
+  });
+
   it("merges allOf branches unconditionally", () => {
     const { base, branches } = resolveRootUnion({
       type: "object",
@@ -408,4 +439,41 @@ describe("resolveRootUnion", () => {
       expect(declaresAnyFields(undefined)).toBe(false);
     });
   });
+
+  describe("selectBranchIndex", () => {
+    const { branches } = resolveRootUnion({
+      type: "object",
+      anyOf: [EMAIL, SMS],
+    });
+
+    it("names the branch whose discriminator the values carry", () => {
+      expect(selectBranchIndex(branches, { kind: "sms" })).toBe(1);
+    });
+
+    it("reports none when the values identify nothing", () => {
+      expect(selectBranchIndex(branches, {})).toBeNull();
+      expect(selectBranchIndex(branches, { kind: "other" })).toBeNull();
+    });
+
+    it("reports none when two branches match", () => {
+      // An ambiguous answer is worse than none: the caller falls back to the
+      // first branch, where the picker and the values at least agree.
+      const ambiguous = resolveRootUnion({
+        type: "object",
+        anyOf: [EMAIL, { ...EMAIL, properties: { ...EMAIL.properties } }],
+      }).branches;
+      expect(selectBranchIndex(ambiguous, { kind: "email" })).toBeNull();
+    });
+
+    it("reports none for a branch that pins nothing", () => {
+      const unpinned = resolveRootUnion({
+        type: "object",
+        anyOf: [
+          { type: "object", properties: { a: { type: "string" } } },
+          { type: "object", properties: { b: { type: "string" } } },
+        ],
+      }).branches;
+      expect(selectBranchIndex(unpinned, { a: "x" })).toBeNull();
+    });
+  });
 });

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

@@ -410,6 +410,28 @@ describe("root composition (#2123)", () => {
     ).toEqual({ merged: "m" });
   });
 
+  it("seeds the branch the known values identify, not the first", () => {
+    // What the App deep link does: seed defaults, then overlay its `appArgs`.
+    // Seeding branch 0 underneath branch 1's args would leave `address` in the
+    // submitted arguments, invisible to a form showing the SMS branch.
+    expect(collectSchemaDefaults(UNION, { kind: "sms" })).toEqual({
+      kind: "sms",
+    });
+  });
+
+  it("accepts a required field pinned to null", () => {
+    // `const: null` admits null and nothing else, so seeding it must not leave
+    // submit disabled on a value the user cannot change.
+    const schema: InspectorFormSchema = {
+      type: "object",
+      properties: { nothing: { const: null } },
+      required: ["nothing"],
+    };
+    const values = collectSchemaDefaults(schema);
+    expect(values).toEqual({ nothing: null });
+    expect(hasMissingRequiredFields(schema, values)).toBe(false);
+  });
+
   it("blocks submission while no branch is satisfied", () => {
     expect(hasMissingRequiredFields(UNION, {})).toBe(true);
     expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true);

clients/web/src/utils/jsonUtils.ts

@@ -2,7 +2,10 @@ import {
   admitsNull,
   normalizeNullableUnion,
 } from "@inspector/core/json/nullableUnion.js";
-import { resolveRootUnion } from "@inspector/core/json/rootUnion.js";
+import {
+  resolveRootUnion,
+  selectBranchIndex,
+} from "@inspector/core/json/rootUnion.js";
 
 export type JsonValue =
   | string
@@ -124,14 +127,23 @@ export function getDataType(value: JsonValue): DataType {
  */
 export function collectSchemaDefaults(
   schema: InspectorFormSchema,
+  knownValues: Record<string, unknown> = {},
 ): Record<string, unknown> {
   // Seed from the shape the form actually renders: root `allOf` merged in, and
-  // for a root union the branch the picker starts on (#2123). Seeding every
+  // for a root union the branch the picker will open on (#2123). Seeding every
   // branch would put fields of shapes the call is not making into the
   // arguments; seeding none would leave the branch's defaults — its
   // discriminator `const` among them — displayed but never submitted.
+  //
+  // `knownValues` is for a caller that already holds arguments it is about to
+  // overlay on these defaults, as the App deep link does with its `appArgs`.
+  // Those values can name a branch other than the first through its
+  // discriminator, and seeding the first branch's defaults underneath them
+  // would leave another shape's fields in the submitted arguments, invisible
+  // to a form showing the branch the values actually identify.
   const { base, branches } = resolveRootUnion(schema);
-  const properties = (branches[0]?.schema ?? base).properties ?? {};
+  const selected = selectBranchIndex(branches, knownValues) ?? 0;
+  const properties = (branches[selected]?.schema ?? base).properties ?? {};
   const result: Record<string, unknown> = {};
   for (const [fieldName, rawSchema] of Object.entries(properties)) {
     // Collapse a nullable union first, for the same reason `SchemaForm` does:

core/json/nullableUnion.ts

@@ -372,6 +372,13 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
   if (nullExcludedBySiblings(schema)) {
     return false;
   }
+  // `const: null` admits null and nothing else, whatever the schema says (or
+  // omits) about `type`. Without this a required field pinned to null is seeded
+  // with the only value it accepts and then reported as missing, leaving submit
+  // permanently disabled on a form the user cannot change (#2123).
+  if (schema.const === null) {
+    return true;
+  }
   // Applicators this module does not evaluate. `not: { type: "null" }` rules
   // null out; `allOf` can add a member that does; and `oneOf` requires
   // **exactly one** branch to match, so a null branch does not by itself mean

core/json/rootUnion.ts

@@ -229,9 +229,20 @@ function mergeProperty(
  * - **It is a `$ref`.** The referent is not resolved by this module, so its
  *   constraints are unknown rather than absent.
  */
+const MERGEABLE_KEYWORDS = new Set(["type", "properties", "required"]);
+
 function isFlattenable(member: unknown): boolean {
   const branch = toBranch(member);
-  return branch !== null && branch.$ref === undefined;
+  if (branch === null || !admitsObject(branch)) return false;
+  // `mergeBranch` applies `properties` and `required` and nothing else, so a
+  // member stating anything further would have that constraint erased along
+  // with the keyword — a nested `anyOf`, a `not`, an `additionalProperties`,
+  // a `$ref` whose referent is not resolved here. Only the keywords the merge
+  // actually carries, plus annotations that constrain nothing, are accepted.
+  return Object.keys(branch).every(
+    (keyword) =>
+      MERGEABLE_KEYWORDS.has(keyword) || ANNOTATION_KEYWORDS.has(keyword),
+  );
 }
 
 /** Whether any property declaration of `branch` conflicts with the base's. */
@@ -391,20 +402,21 @@ export function resolveRootUnion<T extends RootUnionSchema>(
   // branch would otherwise be merged against a base whose constraints are not
   // all known.
   const allOfMembers = schema.allOf ?? [];
-  const flattenable =
-    allOfMembers.every(isFlattenable) &&
-    allOfMembers.every(
-      (member) =>
-        !conflictsWithBase(schema, toBranch(member) as RootUnionSchema),
-    );
-  if (!flattenable) {
-    return { base: schema as ResolvedSchema<T>, branches: [] };
+  let merged = schema as ResolvedSchema<T>;
+  for (const member of allOfMembers) {
+    const branch = toBranch(member);
+    // Each member is checked against what has been merged **so far**, not
+    // against the original root: two members declaring `x.minimum` as 10 and 0
+    // agree with a root that declares neither, while contradicting each other.
+    if (
+      branch === null ||
+      !isFlattenable(member) ||
+      conflictsWithBase(merged, branch)
+    ) {
+      return { base: schema as ResolvedSchema<T>, branches: [] };
+    }
+    merged = mergeBranch(merged, branch);
   }
-
-  const merged = allOfMembers.reduce<ResolvedSchema<T>>(
-    (acc, member) => mergeBranch(acc, toBranch(member) as RootUnionSchema),
-    schema as ResolvedSchema<T>,
-  );
   const base = withoutComposition(merged);
 
   if (schema.oneOf !== undefined && schema.anyOf !== undefined) {
@@ -438,3 +450,38 @@ export function resolveRootUnion<T extends RootUnionSchema>(
       })),
   };
 }
+
+/**
+ * The index of the branch a set of values already identifies, or `null` when
+ * they identify none uniquely.
+ *
+ * A discriminated union pins its discriminator with `const`, so values carrying
+ * one name the branch they belong to. Shared because more than one caller has
+ * to reach the same answer: the web form opens its picker on that branch, and
+ * the defaults seeded before the values are overlaid must belong to the same
+ * one, or the arguments carry a shape the picker is not showing.
+ */
+export function selectBranchIndex<T extends RootUnionSchema>(
+  branches: RootUnionBranch<T>[],
+  values: Record<string, unknown>,
+): number | null {
+  const matches: number[] = [];
+  branches.forEach((branch, index) => {
+    const pinned = Object.entries(propertiesOf(branch.schema) ?? {})
+      .map(
+        ([name, schema]) =>
+          [
+            name,
+            (toBranch(schema) as { const?: unknown } | null)?.const,
+          ] as const,
+      )
+      .filter(([, constValue]) => constValue !== undefined);
+    if (
+      pinned.length > 0 &&
+      pinned.every(([name, constValue]) => values[name] === constValue)
+    ) {
+      matches.push(index);
+    }
+  });
+  return matches.length === 1 ? matches[0] : null;
+}