From 7e4af5ed34f8b58403e3398f99f6f9982f3c5b30 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 02:24:17 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=204?= =?UTF-8?q?=20=E2=80=94=20allOf=20faithfulness,=20const=20null,=20branch-a?= =?UTF-8?q?ware=20seeding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../groups/SchemaForm/SchemaForm.tsx | 39 ++-------- .../views/InspectorView/InspectorView.tsx | 10 ++- clients/web/src/test/core/rootUnion.test.ts | 68 +++++++++++++++++ clients/web/src/utils/jsonUtils.test.ts | 22 ++++++ clients/web/src/utils/jsonUtils.ts | 18 ++++- core/json/nullableUnion.ts | 7 ++ core/json/rootUnion.ts | 75 +++++++++++++++---- 7 files changed, 188 insertions(+), 51 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index bb162576..5974fa4c 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/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, -): 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 diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index fef3d470..74e2f803 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/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 diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index 21794afd..e1a9acf7 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/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(); + }); + }); }); diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index f788b5dd..7f3c1a0c 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/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); diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 0e29195e..c078c036 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/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 = {}, ): Record { // 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 = {}; for (const [fieldName, rawSchema] of Object.entries(properties)) { // Collapse a nullable union first, for the same reason `SchemaForm` does: diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index e4cc8b24..9dad6f8e 100644 --- a/core/json/nullableUnion.ts +++ b/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 diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 52fd029a..fff5c7c7 100644 --- a/core/json/rootUnion.ts +++ b/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( // 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, branches: [] }; + let merged = schema as ResolvedSchema; + 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, branches: [] }; + } + merged = mergeBranch(merged, branch); } - - const merged = allOfMembers.reduce>( - (acc, member) => mergeBranch(acc, toBranch(member) as RootUnionSchema), - schema as ResolvedSchema, - ); const base = withoutComposition(merged); if (schema.oneOf !== undefined && schema.anyOf !== undefined) { @@ -438,3 +450,38 @@ export function resolveRootUnion( })), }; } + +/** + * 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( + branches: RootUnionBranch[], + values: Record, +): 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; +}