From 775fec0f6a933886832ba420b8ec7a868a9bccc4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 04:23:14 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2010?= =?UTF-8?q?=20=E2=80=94=20structural=20const=20match,=20deep-link=20consta?= =?UTF-8?q?nts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - selectBranchIndex compares a discriminator structurally: a `const` may be an object or array, and deep-link arguments arrive as freshly parsed instances that could never be reference-equal to the schema's own. - New applySchemaConstants, applied by InspectorView AFTER the deep link's `appArgs` are spread over the seeded defaults. The form renders a pinned field read-only, so a link disagreeing with one would otherwise auto-open an App with a hidden value contradicting the shape on screen. Signed-off-by: cliffhall --- .../views/InspectorView/InspectorView.tsx | 20 ++++++++----- clients/web/src/test/core/rootUnion.test.ts | 19 ++++++++++++ clients/web/src/utils/jsonUtils.test.ts | 23 ++++++++++++++ clients/web/src/utils/jsonUtils.ts | 30 +++++++++++++++++++ core/json/rootUnion.ts | 7 ++++- 5 files changed, 91 insertions(+), 8 deletions(-) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 74e2f803..6e845dba 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -103,7 +103,11 @@ import { correlatedFetchStatusById, revealableMessageIds, } from "../../../utils/correlateTransportErrors"; -import { collectSchemaDefaults, toFormSchema } from "../../../utils/jsonUtils"; +import { + applySchemaConstants, + collectSchemaDefaults, + toFormSchema, +} from "../../../utils/jsonUtils"; import { MONITOR_COLUMN_ANIM_MS } from "./monitorColumnAnimation"; const SORT_DEFAULT: SortDirection = "newest-first"; @@ -1040,13 +1044,15 @@ export function InspectorView({ // 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) ?? {}, - deepLink.appArgs ?? {}, - ), + // …and the schema's constants are re-applied *after* the overlay: a field + // the form renders read-only cannot be corrected by the user, so a deep + // link disagreeing with one would otherwise auto-open with a hidden value + // contradicting the shape on screen. + const appFormSchema = toFormSchema(target.inputSchema) ?? {}; + const formValues = applySchemaConstants(appFormSchema, { + ...collectSchemaDefaults(appFormSchema, deepLink.appArgs ?? {}), ...deepLink.appArgs, - }; + }); // Seed the selection directly rather than routing through // AppsScreen.handleSelect. This deliberately bypasses handleSelect's // no-input-app auto-launch: a deep link must never invoke a tool against diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index fd563ac6..f98d8903 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -695,6 +695,25 @@ describe("resolveRootUnion", () => { expect(selectBranchIndex(undiscriminated, {})).toBeNull(); }); + it("matches an object-valued discriminator structurally", () => { + // Deep-link arguments are freshly parsed instances, never `===` the + // schema's own constant. + const objectPinned = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { tag: { const: { a: 1 } }, x: { type: "string" } }, + }, + { + type: "object", + properties: { tag: { const: { a: 2 } }, y: { type: "string" } }, + }, + ], + }).branches; + expect(selectBranchIndex(objectPinned, { tag: { a: 2 } })).toBe(1); + }); + it("reports none when the values identify nothing", () => { expect(selectBranchIndex(branches, {})).toBeNull(); expect(selectBranchIndex(branches, { kind: "other" })).toBeNull(); diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 7f3c1a0c..54c2fa25 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -6,6 +6,7 @@ import { getValueAtPath, collectSchemaDefaults, hasMissingRequiredFields, + applySchemaConstants, } from "./jsonUtils"; import type { InspectorFormSchema } from "./jsonUtils"; @@ -432,6 +433,28 @@ describe("root composition (#2123)", () => { expect(hasMissingRequiredFields(schema, values)).toBe(false); }); + it("re-applies a branch's constants over conflicting supplied values", () => { + // A read-only field cannot be corrected by the user, so a deep link + // disagreeing with a `const` must not survive into the submitted arguments. + expect(applySchemaConstants(UNION, { kind: "sms", note: "hi" })).toEqual({ + kind: "sms", + note: "hi", + }); + expect(applySchemaConstants(UNION, { kind: "nonsense" })).toEqual({ + kind: "email", + }); + }); + + it("leaves values alone when nothing is pinned", () => { + const values = { a: 1 }; + expect( + applySchemaConstants( + { type: "object", properties: { a: { type: "number" } } }, + values, + ), + ).toBe(values); + }); + 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 c078c036..4145e4a9 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -174,6 +174,36 @@ export function collectSchemaDefaults( return result; } +/** + * Overwrite every `const`-pinned field with the value its schema fixes. + * + * The form renders such a field read-only, so a value disagreeing with it can + * only have come from outside the form — an App deep link's `appArgs`, which + * are spread over the seeded defaults and would otherwise leave the arguments + * claiming a shape the user is being shown the opposite of (#2123). Apply this + * *after* any such overlay. + * + * The branch is chosen the same way {@link collectSchemaDefaults} chooses it, + * from the values themselves, so the constants applied belong to the shape the + * form will display. + */ +export function applySchemaConstants( + schema: InspectorFormSchema, + values: Record, +): Record { + const { base, branches } = resolveRootUnion(schema); + const selected = selectBranchIndex(branches, values) ?? 0; + const properties = (branches[selected]?.schema ?? base).properties ?? {}; + const pinned = Object.entries(properties).filter( + ([, fieldSchema]) => fieldSchema.const !== undefined, + ); + if (pinned.length === 0) return values; + return Object.fromEntries([ + ...Object.entries(values), + ...pinned.map(([name, fieldSchema]) => [name, fieldSchema.const]), + ]); +} + /** * Whether any of the schema's required top-level fields is missing a value in * `values` (absent, null, or empty string). Used to gate a form's submit diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index f18fa748..04f6a561 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -605,7 +605,12 @@ export function selectBranchIndex( const supplied = pinned.filter(([name]) => values[name] !== undefined); if ( supplied.length > 0 && - supplied.every(([name, constValue]) => values[name] === constValue) + // Structural, not reference: a `const` may be an object or an array, and + // 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); }