From 0be450a911f4a603d07c17522a2eb2fa325b6618 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 07:56:15 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2022?= =?UTF-8?q?=20=E2=80=94=20merge=20deep-link=20args=20with=20defaults=20per?= =?UTF-8?q?=20level?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The App deep link seeded defaults and spread its `appArgs` over them, which replaces a nested object wholesale: `{ config: { kind: "sms" } }` discarded the nested SMS branch's own defaults, which the form then displayed while the submitted arguments omitted them. New seedSchemaValues merges the two per level, picking each nested branch from the nested values, and the supplied value still wins wherever the two meet. Signed-off-by: cliffhall --- .../views/InspectorView/InspectorView.tsx | 13 ++-- clients/web/src/utils/jsonUtils.test.ts | 60 +++++++++++++++++++ clients/web/src/utils/jsonUtils.ts | 54 +++++++++++++++++ 3 files changed, 122 insertions(+), 5 deletions(-) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 6e845dba..43749729 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -105,7 +105,7 @@ import { } from "../../../utils/correlateTransportErrors"; import { applySchemaConstants, - collectSchemaDefaults, + seedSchemaValues, toFormSchema, } from "../../../utils/jsonUtils"; import { MONITOR_COLUMN_ANIM_MS } from "./monitorColumnAnimation"; @@ -1049,10 +1049,13 @@ export function InspectorView({ // 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, - }); + const formValues = applySchemaConstants( + appFormSchema, + // Merged per level, not with one shallow spread: a nested object in the + // args would otherwise replace the whole seeded object, discarding the + // nested defaults the form goes on displaying. + seedSchemaValues(appFormSchema, 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/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 228f81f3..62d0e123 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -7,6 +7,7 @@ import { collectSchemaDefaults, hasMissingRequiredFields, applySchemaConstants, + seedSchemaValues, } from "./jsonUtils"; import type { InspectorFormSchema } from "./jsonUtils"; @@ -488,6 +489,65 @@ describe("root composition (#2123)", () => { ).toEqual({ config: { kind: "email" } }); }); + describe("seedSchemaValues", () => { + const NESTED: InspectorFormSchema = { + type: "object", + properties: { + config: { + type: "object", + anyOf: [ + { + type: "object", + properties: { + kind: { type: "string", const: "email" }, + retries: { type: "number", default: 1 }, + }, + }, + { + type: "object", + properties: { + kind: { type: "string", const: "sms" }, + retries: { type: "number", default: 3 }, + }, + }, + ], + }, + }, + }; + + it("keeps a nested branch's defaults beside the supplied values", () => { + // A shallow spread would replace the whole `config` object, so the SMS + // branch's `retries` would be displayed by the form and never submitted. + expect(seedSchemaValues(NESTED, { config: { kind: "sms" } })).toEqual({ + config: { kind: "sms", retries: 3 }, + }); + }); + + it("lets the supplied value win where the two meet", () => { + expect( + seedSchemaValues(NESTED, { config: { kind: "sms", retries: 9 } }), + ).toEqual({ config: { kind: "sms", retries: 9 } }); + }); + + it("does not treat an array as a nested object", () => { + const withArray: InspectorFormSchema = { + type: "object", + properties: { items: { type: "array" } }, + }; + expect(seedSchemaValues(withArray, { items: [1, 2] })).toEqual({ + items: [1, 2], + }); + }); + + it("keeps a supplied argument named __proto__", () => { + const seeded = seedSchemaValues( + { type: "object", properties: {} }, + Object.fromEntries([["__proto__", "kept"]]), + ); + expect(Object.hasOwn(seeded, "__proto__")).toBe(true); + }); + }); + it("re-applies a nested object's constants", () => { // The overlay replaces the whole nested object rather than merging into it, // so a link naming `{ config: { kind: "sms" } }` would otherwise slip past diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index acf5f0a7..5293c697 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -194,6 +194,60 @@ export function collectSchemaDefaults( return result; } +/** + * Seed a schema's defaults underneath values a caller already holds, merging + * the two **per level** rather than with one shallow spread. + * + * The App deep link is the caller: it holds `appArgs` and needs the defaults + * the form would otherwise display. A shallow `{ ...defaults, ...args }` + * replaces a nested object wholesale, so `{ config: { kind: "sms" } }` discards + * the nested SMS branch's own defaults — which the form then shows while the + * submitted arguments omit them (#2123). Recursing also lets each nested level + * pick its branch from the nested values, rather than from the top-level ones. + * + * The supplied value always wins where the two meet; only what it does not + * mention is seeded. + */ +export function seedSchemaValues( + schema: InspectorFormSchema, + suppliedValues: Record, +): Record { + const { base, branches } = resolveRootUnion(schema); + const selected = selectBranchIndex(branches, suppliedValues) ?? 0; + const effective = branches[selected]?.schema ?? base; + const properties = effective.properties ?? {}; + + const merged: Record = { + ...collectSchemaDefaults(effective, suppliedValues), + }; + for (const [name, value] of Object.entries(suppliedValues)) { + const fieldSchema = normalizeNullableUnion(properties[name] ?? {}); + const mergedValue = + isPlainObject(value) && + fieldSchema.type === "object" && + declaresAnyFields(fieldSchema) + ? seedSchemaValues( + fieldSchema, + // The nested defaults are re-derived by the recursion, so only the + // supplied half is passed down. + value, + ) + : value; + Object.defineProperty(merged, name, { + value: mergedValue, + writable: true, + enumerable: true, + configurable: true, + }); + } + return merged; +} + +/** Whether a value is a plain object a nested schema could describe. */ +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + /** * Overwrite every `const`-pinned field with the value its schema fixes. *