From b0b252a5c02e28ee29f48d67ee00fdcb8bf243d3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 26 Aug 2026 16:08:24 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Copilot=20review=20round=2041?= =?UTF-8?q?=20=E2=80=94=20const=20does=20not=20demand=20a=20property,=20re?= =?UTF-8?q?quired-only=20branches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `const` constrains a property that is PRESENT; it neither requires the property nor acts as a default. It is now supplied automatically only where the schema also requires the field — in `collectSchemaDefaults`, `applySchemaConstants` and the TUI's initial values alike — so an optional `dryRun: { const: true }` no longer turns a valid `{}` call into one that asks the server to do something. A value that IS present is still corrected to the constant, whatever supplied it, and the field stays read-only. - Offerability is judged on the base+member merge rather than the raw member, so `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` over root-declared properties is offered. It was declined, which left the gate checking the base alone and accepting `{}` — which that schema rejects. - The TUI's branch sections keep the schema's own `required` for the seeding decision while still rendering every branch field non-required, which is what keeps a static form submittable. Signed-off-by: cliffhall --- README.md | 2 +- clients/tui/__tests__/schemaToForm.test.ts | 21 ++++++ clients/tui/src/utils/schemaToForm.ts | 69 ++++++++++++++++--- .../groups/SchemaForm/SchemaForm.test.tsx | 2 + clients/web/src/test/core/rootUnion.test.ts | 18 +++++ clients/web/src/utils/jsonUtils.test.ts | 23 ++++++- clients/web/src/utils/jsonUtils.ts | 32 +++++++-- core/json/rootUnion.ts | 22 ++++-- 8 files changed, 167 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 139272c3..1001d9ee 100644 --- a/README.md +++ b/README.md @@ -296,7 +296,7 @@ The **TUI** had the same gap and is worth checking against the same server (`--t The 2026-07-28 revision makes this shape explicitly legal: `type: "object"` is required at the root, and beyond that "any JSON Schema 2020-12 keyword may appear alongside `type`, including composition keywords (`oneOf`, `anyOf`, `allOf`, `not`)". -Open the Tools tab and select `echo`. Above the fields is a **Variant** picker listing the union's alternatives — labelled from each branch's `title`, else its discriminator `const`, else its position — and choosing one swaps in that branch's fields with the discriminator already filled in. +Open the Tools tab and select `echo`. Above the fields is a **Variant** picker listing the union's alternatives — labelled from each branch's `title`, else its discriminator `const`, else its position — and choosing one swaps in that branch's fields with the discriminator already filled in. A field the schema pins with `const` renders read-only, and is filled in automatically only where the schema also **requires** it: `const` constrains a value that is present rather than demanding one, so an optional pinned field stays omittable. The two tools show the two halves of the old behavior. On the broken build `echo` rendered its root `message` and **nothing from either branch**, so it could only ever be called with half its arguments; `get_weather`, whose fields live entirely on its `oneOf`, rendered **nothing but the Execute Tool button** — no picker, no fields, not even the raw-JSON editor a union-typed _property_ falls back to ([#2123](https://github.com/modelcontextprotocol/inspector/issues/2123)). diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index 521ba528..b245132e 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -555,6 +555,7 @@ describe("schemaToForm", () => { { type: "object", properties: { v: { type: "string", const: "a", default: "b" } }, + required: ["v"], }, "const_tool", ); @@ -565,6 +566,26 @@ describe("schemaToForm", () => { }); }); + it("leaves an optional const unfilled", () => { + // `const` constrains a present value; it does not require the property + // or act as a default, so an optional one must stay omittable. + const form = schemaToForm( + { + type: "object", + properties: { dryRun: { type: "boolean", const: true } }, + }, + "optional_const", + ); + const field = form.sections[0]!.fields[0] as { + initialValue?: unknown; + options?: unknown[]; + }; + expect(field.initialValue).toBeUndefined(); + // The single option is still offered to a user who wants it. + expect(field.options).toEqual([{ label: "true", value: "true" }]); + expect(decodeFormValues({ type: "object" }, {})).toEqual({}); + }); + it("renders a branch's specialization of a root property in its section", () => { const form = schemaToForm( { diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 1c276dbd..a86f3d18 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -156,6 +156,13 @@ function labelled(property: unknown, name: string): unknown { return { title: name, ...property }; } +/** A schema's `required`, as the list of strings it should be. */ +function requiredOf(schema: { required?: unknown }): string[] { + return Array.isArray(schema.required) + ? schema.required.filter((name): name is string => typeof name === "string") + : []; +} + /** The `const` a property schema pins its value to, if any. */ function constOf(schema: unknown): unknown { if (typeof schema !== "object" || schema === null) return undefined; @@ -227,6 +234,9 @@ export function schemaToForm( // says: only one alternative applies to a call, so requiring them would build // a form that can never be submitted. branches.forEach((branch, index) => { + const byRealName = Object.fromEntries( + branchFields(base, branches, index).map((name) => [name, true]), + ); const properties = Object.fromEntries( branchFields(base, branches, index).map((name) => [ branchFieldName(prefix, index, name), @@ -239,7 +249,17 @@ export function schemaToForm( ); sections.push({ title: branch.label, - fields: buildFields({ properties }), + fields: buildFields( + { + properties, + // Under their prefixed names, so the constant seeding below can tell + // which of this branch's fields the schema actually requires. + required: requiredOf(branch.schema) + .filter((name) => Object.hasOwn(byRealName, name)) + .map((name) => branchFieldName(prefix, index, name)), + }, + { optional: true }, + ), }); }); @@ -270,7 +290,7 @@ export function decodeFormValues( ): Record { const { base, branches } = resolveRootUnion(schema ?? {}); if (branches.length === 0) { - return applyConstants(base.properties ?? {}, values); + return applyConstants(base.properties ?? {}, values, requiredOf(base)); } const { variant, prefix } = generatedNames(base, branches); @@ -298,7 +318,11 @@ export function decodeFormValues( .filter(([, value]) => value !== undefined), ]); /* v8 ignore next -- an offerable branch always carries properties */ - return applyConstants(branch.schema.properties ?? {}, decoded); + return applyConstants( + branch.schema.properties ?? {}, + decoded, + requiredOf(branch.schema), + ); } /** @@ -320,13 +344,24 @@ function selectedBranchIndex( : 0; } -/** Overwrite every `const`-pinned field with the value its schema fixes. */ +/** + * Overwrite every `const`-pinned field with the value its schema fixes — + * where the field is PRESENT, or where the schema also requires it. + * + * `const` constrains a present value; it neither requires the property nor + * acts as a default. Restoring an optional one the user left blank would send + * it on every call. + */ function applyConstants( properties: Record, values: Record, + required: string[], ): Record { const pinned = Object.entries(properties).filter( - ([, schema]) => constOf(schema) !== undefined, + ([name, schema]) => + constOf(schema) !== undefined && + (required.includes(name) || + (Object.hasOwn(values, name) && values[name] !== undefined)), ); if (pinned.length === 0) return values; return Object.fromEntries([ @@ -335,8 +370,19 @@ function applyConstants( ]); } -/** Build the ink-form fields for one already-flattened object schema. */ -function buildFields(schema: JsonSchemaObject): FormField[] { +/** + * Build the ink-form fields for one already-flattened object schema. + * + * `optional` renders every field non-required whatever the schema says, for a + * branch section: only one alternative applies to a call, so demanding all of + * them would build a form that can never be submitted. The schema's own + * `required` is still read — it decides which constants are pre-filled, and + * `missingRequiredFields` enforces the real requirement at submit. + */ +function buildFields( + schema: JsonSchemaObject, + { optional = false }: { optional?: boolean } = {}, +): FormField[] { const fields: FormField[] = []; const properties = schema.properties || {}; // `Array.isArray`, not `|| []`: a nonconforming server can send @@ -360,7 +406,7 @@ function buildFields(schema: JsonSchemaObject): FormField[] { const baseField = { name: key, label: property.title || key, - required: required.includes(key), + required: optional ? false : required.includes(key), }; let field: FormField; @@ -374,13 +420,18 @@ function buildFields(schema: JsonSchemaObject): FormField[] { fields.push({ type: "select", ...baseField, + // Only a REQUIRED constant is pre-filled. `const` constrains a present + // value; it neither requires the property nor acts as a default, so an + // optional `dryRun: { const: true }` must stay omittable rather than + // being sent on every call. The single option is still there for a user + // who wants it. + ...(required.includes(key) ? { initialValue: String(pinned) } : {}), // Never required: the one option may legitimately be the empty string, // which ink-form's required gate can never accept — submission would // not even reach `decodeFormValues`, which reapplies the constant. The // value is fixed by the schema, and `missingRequiredFields` still // validates the decoded call. required: false, - initialValue: String(pinned), options: [{ label: String(pinned), value: String(pinned) }], } as FormField); continue; diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 66a529bb..b0c24438 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2344,6 +2344,7 @@ describe("SchemaForm multiline strings (#2042)", () => { a: { type: "string", title: "A" }, version: { type: "string", const: "2" }, }, + required: ["version"], }} values={{}} onChange={onChange} @@ -2402,6 +2403,7 @@ describe("SchemaForm multiline strings (#2042)", () => { kind: { type: "string", const: value }, note: { type: "string", title: "Note" }, }, + required: ["kind"], }); const { rerender } = renderWithMantine( { }); }); + it("offers a branch that only adds required names over the root's fields", () => { + // `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` is an + // ordinary way to say "one of these two". Judging the member on its own + // properties declined it, leaving the gate checking the base alone and + // accepting `{}` — which the schema rejects. + const { branches } = resolveRootUnion({ + type: "object", + properties: { email: { type: "string" }, phone: { type: "string" } }, + anyOf: [ + { type: "object", required: ["email"] }, + { type: "object", required: ["phone"] }, + ], + }); + expect(branches).toHaveLength(2); + expect(branches[0].schema.required).toEqual(["email"]); + expect(branches[1].schema.required).toEqual(["phone"]); + }); + describe("oneOf exclusivity", () => { it("offers a discriminated oneOf", () => { expect( diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 10d7870b..f7de8092 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -375,15 +375,32 @@ describe("root composition (#2123)", () => { expect(collectSchemaDefaults(UNION)).not.toHaveProperty("phone"); }); - it("seeds a const property on an ordinary schema too", () => { + it("seeds a required const property on an ordinary schema too", () => { expect( collectSchemaDefaults({ type: "object", properties: { version: { type: "string", const: "1" } }, + required: ["version"], }), ).toEqual({ version: "1" }); }); + it("leaves an optional const out", () => { + // `const` constrains a present value; it does not require the property or + // act as a default, so seeding it would turn a valid `{}` call into one + // that asks the server to do something. + const optional: InspectorFormSchema = { + type: "object", + properties: { dryRun: { type: "boolean", const: true } }, + }; + expect(collectSchemaDefaults(optional)).toEqual({}); + expect(applySchemaConstants(optional, {})).toEqual({}); + // …but a value that IS present must be the one the schema fixes. + expect(applySchemaConstants(optional, { dryRun: false })).toEqual({ + dryRun: true, + }); + }); + it("prefers a const over a conflicting default", () => { // `default` is an annotation, not a constraint, so a schema may advertise // one its own `const` rejects — seeding it would submit an invalid value @@ -394,6 +411,7 @@ describe("root composition (#2123)", () => { properties: { v: { type: "string", const: "a", default: "b" }, }, + required: ["v"], }), ).toEqual({ v: "a" }); }); @@ -454,6 +472,7 @@ describe("root composition (#2123)", () => { properties: Object.fromEntries([ ["__proto__", { type: "string", const: "kept" }], ]), + required: ["__proto__"], }); expect(Object.hasOwn(seeded, "__proto__")).toBe(true); }); @@ -474,6 +493,7 @@ describe("root composition (#2123)", () => { kind: { type: "string", const: "email" }, address: { type: "string" }, }, + required: ["kind"], }, { type: "object", @@ -481,6 +501,7 @@ describe("root composition (#2123)", () => { kind: { type: "string", const: "sms" }, phone: { type: "string" }, }, + required: ["kind"], }, ], }, diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index f2f9a56c..24a31e91 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -145,7 +145,17 @@ export function collectSchemaDefaults( // to a form showing the branch the values actually identify. const { base, branches } = resolveRootUnion(schema); const selected = selectBranchIndex(branches, knownValues) ?? 0; - const properties = (branches[selected]?.schema ?? base).properties ?? {}; + const effective = branches[selected]?.schema ?? base; + const properties = effective.properties ?? {}; + // `const` constrains a property only when it is PRESENT — it neither requires + // the property nor acts as a default. So it is supplied automatically for a + // REQUIRED field, whose only submittable value was never in doubt and which + // the form renders read-only, and left alone otherwise: seeding an optional + // `dryRun: { const: true }` would turn a valid `{}` call into one that asks + // the server to do something. + const requiredNames = Array.isArray(effective.required) + ? effective.required + : []; const result: Record = {}; // `properties` is a JSON record, so `__proto__` is a legal field name that a // plain assignment would drop into the legacy prototype setter rather than @@ -164,10 +174,10 @@ export function collectSchemaDefaults( // without this the form would *display* a hoisted default that never // reached the seeded values — the field would submit empty (#1928). const fieldSchema = normalizeNullableUnion(rawSchema); - if (fieldSchema.const !== undefined) { - // `const` is a one-value enumeration, so the only submittable value is - // already known — seeding it spares the user hand-typing a discriminator - // the schema has fixed (#2123). + if (fieldSchema.const !== undefined && requiredNames.includes(fieldName)) { + // `const` is a one-value enumeration, so a required field's only + // submittable value is already known — seeding it spares the user + // hand-typing a discriminator the schema has fixed (#2123). // // It outranks `default`, which JSON Schema defines as an annotation // rather than a constraint: a schema may advertise a default its own @@ -267,13 +277,21 @@ export function applySchemaConstants( ): Record { const { base, branches } = resolveRootUnion(schema); const selected = selectBranchIndex(branches, values) ?? 0; - const properties = (branches[selected]?.schema ?? base).properties ?? {}; + const effective = branches[selected]?.schema ?? base; + const properties = effective.properties ?? {}; + const required = Array.isArray(effective.required) ? effective.required : []; const corrections: [string, unknown][] = []; for (const [name, rawSchema] of Object.entries(properties)) { const fieldSchema = normalizeNullableUnion(rawSchema); if (fieldSchema.const !== undefined) { - corrections.push([name, fieldSchema.const]); + // Corrected where the property is PRESENT — whatever supplied it, the + // schema fixes the value — and inserted only where the schema also + // requires it. An optional `const` the caller left out stays out: + // `const` constrains a present value, it does not demand one. + if (Object.hasOwn(values, name) || required.includes(name)) { + corrections.push([name, fieldSchema.const]); + } continue; } // Recurse: a nested object renders its own read-only fields, and the diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 279cd1a7..7921824c 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -175,9 +175,23 @@ function propertiesOf(schema: RootUnionSchema): Record { * `{ type: "string", properties: {…} }` member can never match — rendering it * as a fillable form would offer a call that cannot be valid. */ -function isOfferable(branch: RootUnionSchema): boolean { - if (!hasReadableProperties(branch) || !admitsObject(branch)) return false; - const properties = Object.values(propertiesOf(branch)); +function isOfferable( + branch: RootUnionSchema, + merged: RootUnionSchema, +): boolean { + // The MEMBER decides whether the shape is readable at all… + if (branch.properties !== undefined && !hasReadableProperties(branch)) { + return false; + } + if (!admitsObject(branch)) return false; + + // …and the MERGE decides whether there is anything to render. A member may + // legitimately declare no properties of its own and only add `required` over + // names the root already declares — `anyOf: [{ required: ["email"] }, …]` is + // a perfectly ordinary way to say "one of these two" — and judging it on its + // own properties would decline it, leaving the gate checking the base alone + // and accepting `{}`, which the schema rejects. + const properties = Object.values(propertiesOf(merged)); return ( properties.length > 0 && // Every value has to be something a renderer can read AND something a @@ -679,7 +693,7 @@ export function resolveRootUnion( branches.some( (branch) => branch === null || - !isOfferable(branch) || + !isOfferable(branch, mergeBranch(base, branch)) || // The same faithfulness test the `allOf` fold applies: a member // carrying a constraint the merge does not copy — a nested `allOf`, a // `not`, a `$ref` — would have it erased along with the union keyword,