Skip to content

Commit b0b252a

Browse files
committed
fix: address Copilot review round 41 — const does not demand a property, required-only branches
- `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 <cliff@futurescale.com>
1 parent 8f935ad commit b0b252a

8 files changed

Lines changed: 167 additions & 22 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ The **TUI** had the same gap and is worth checking against the same server (`--t
296296

297297
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`)".
298298

299-
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.
299+
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.
300300

301301
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)).
302302

‎clients/tui/__tests__/schemaToForm.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,7 @@ describe("schemaToForm", () => {
555555
{
556556
type: "object",
557557
properties: { v: { type: "string", const: "a", default: "b" } },
558+
required: ["v"],
558559
},
559560
"const_tool",
560561
);
@@ -565,6 +566,26 @@ describe("schemaToForm", () => {
565566
});
566567
});
567568

569+
it("leaves an optional const unfilled", () => {
570+
// `const` constrains a present value; it does not require the property
571+
// or act as a default, so an optional one must stay omittable.
572+
const form = schemaToForm(
573+
{
574+
type: "object",
575+
properties: { dryRun: { type: "boolean", const: true } },
576+
},
577+
"optional_const",
578+
);
579+
const field = form.sections[0]!.fields[0] as {
580+
initialValue?: unknown;
581+
options?: unknown[];
582+
};
583+
expect(field.initialValue).toBeUndefined();
584+
// The single option is still offered to a user who wants it.
585+
expect(field.options).toEqual([{ label: "true", value: "true" }]);
586+
expect(decodeFormValues({ type: "object" }, {})).toEqual({});
587+
});
588+
568589
it("renders a branch's specialization of a root property in its section", () => {
569590
const form = schemaToForm(
570591
{

‎clients/tui/src/utils/schemaToForm.ts‎

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,13 @@ function labelled(property: unknown, name: string): unknown {
156156
return { title: name, ...property };
157157
}
158158

159+
/** A schema's `required`, as the list of strings it should be. */
160+
function requiredOf(schema: { required?: unknown }): string[] {
161+
return Array.isArray(schema.required)
162+
? schema.required.filter((name): name is string => typeof name === "string")
163+
: [];
164+
}
165+
159166
/** The `const` a property schema pins its value to, if any. */
160167
function constOf(schema: unknown): unknown {
161168
if (typeof schema !== "object" || schema === null) return undefined;
@@ -227,6 +234,9 @@ export function schemaToForm(
227234
// says: only one alternative applies to a call, so requiring them would build
228235
// a form that can never be submitted.
229236
branches.forEach((branch, index) => {
237+
const byRealName = Object.fromEntries(
238+
branchFields(base, branches, index).map((name) => [name, true]),
239+
);
230240
const properties = Object.fromEntries(
231241
branchFields(base, branches, index).map((name) => [
232242
branchFieldName(prefix, index, name),
@@ -239,7 +249,17 @@ export function schemaToForm(
239249
);
240250
sections.push({
241251
title: branch.label,
242-
fields: buildFields({ properties }),
252+
fields: buildFields(
253+
{
254+
properties,
255+
// Under their prefixed names, so the constant seeding below can tell
256+
// which of this branch's fields the schema actually requires.
257+
required: requiredOf(branch.schema)
258+
.filter((name) => Object.hasOwn(byRealName, name))
259+
.map((name) => branchFieldName(prefix, index, name)),
260+
},
261+
{ optional: true },
262+
),
243263
});
244264
});
245265

@@ -270,7 +290,7 @@ export function decodeFormValues<T>(
270290
): Record<string, T> {
271291
const { base, branches } = resolveRootUnion(schema ?? {});
272292
if (branches.length === 0) {
273-
return applyConstants(base.properties ?? {}, values);
293+
return applyConstants(base.properties ?? {}, values, requiredOf(base));
274294
}
275295

276296
const { variant, prefix } = generatedNames(base, branches);
@@ -298,7 +318,11 @@ export function decodeFormValues<T>(
298318
.filter(([, value]) => value !== undefined),
299319
]);
300320
/* v8 ignore next -- an offerable branch always carries properties */
301-
return applyConstants(branch.schema.properties ?? {}, decoded);
321+
return applyConstants(
322+
branch.schema.properties ?? {},
323+
decoded,
324+
requiredOf(branch.schema),
325+
);
302326
}
303327

304328
/**
@@ -320,13 +344,24 @@ function selectedBranchIndex(
320344
: 0;
321345
}
322346

323-
/** Overwrite every `const`-pinned field with the value its schema fixes. */
347+
/**
348+
* Overwrite every `const`-pinned field with the value its schema fixes —
349+
* where the field is PRESENT, or where the schema also requires it.
350+
*
351+
* `const` constrains a present value; it neither requires the property nor
352+
* acts as a default. Restoring an optional one the user left blank would send
353+
* it on every call.
354+
*/
324355
function applyConstants<T>(
325356
properties: Record<string, unknown>,
326357
values: Record<string, T>,
358+
required: string[],
327359
): Record<string, T> {
328360
const pinned = Object.entries(properties).filter(
329-
([, schema]) => constOf(schema) !== undefined,
361+
([name, schema]) =>
362+
constOf(schema) !== undefined &&
363+
(required.includes(name) ||
364+
(Object.hasOwn(values, name) && values[name] !== undefined)),
330365
);
331366
if (pinned.length === 0) return values;
332367
return Object.fromEntries([
@@ -335,8 +370,19 @@ function applyConstants<T>(
335370
]);
336371
}
337372

338-
/** Build the ink-form fields for one already-flattened object schema. */
339-
function buildFields(schema: JsonSchemaObject): FormField[] {
373+
/**
374+
* Build the ink-form fields for one already-flattened object schema.
375+
*
376+
* `optional` renders every field non-required whatever the schema says, for a
377+
* branch section: only one alternative applies to a call, so demanding all of
378+
* them would build a form that can never be submitted. The schema's own
379+
* `required` is still read — it decides which constants are pre-filled, and
380+
* `missingRequiredFields` enforces the real requirement at submit.
381+
*/
382+
function buildFields(
383+
schema: JsonSchemaObject,
384+
{ optional = false }: { optional?: boolean } = {},
385+
): FormField[] {
340386
const fields: FormField[] = [];
341387
const properties = schema.properties || {};
342388
// `Array.isArray`, not `|| []`: a nonconforming server can send
@@ -360,7 +406,7 @@ function buildFields(schema: JsonSchemaObject): FormField[] {
360406
const baseField = {
361407
name: key,
362408
label: property.title || key,
363-
required: required.includes(key),
409+
required: optional ? false : required.includes(key),
364410
};
365411

366412
let field: FormField;
@@ -374,13 +420,18 @@ function buildFields(schema: JsonSchemaObject): FormField[] {
374420
fields.push({
375421
type: "select",
376422
...baseField,
423+
// Only a REQUIRED constant is pre-filled. `const` constrains a present
424+
// value; it neither requires the property nor acts as a default, so an
425+
// optional `dryRun: { const: true }` must stay omittable rather than
426+
// being sent on every call. The single option is still there for a user
427+
// who wants it.
428+
...(required.includes(key) ? { initialValue: String(pinned) } : {}),
377429
// Never required: the one option may legitimately be the empty string,
378430
// which ink-form's required gate can never accept — submission would
379431
// not even reach `decodeFormValues`, which reapplies the constant. The
380432
// value is fixed by the schema, and `missingRequiredFields` still
381433
// validates the decoded call.
382434
required: false,
383-
initialValue: String(pinned),
384435
options: [{ label: String(pinned), value: String(pinned) }],
385436
} as FormField);
386437
continue;

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2344,6 +2344,7 @@ describe("SchemaForm multiline strings (#2042)", () => {
23442344
a: { type: "string", title: "A" },
23452345
version: { type: "string", const: "2" },
23462346
},
2347+
required: ["version"],
23472348
}}
23482349
values={{}}
23492350
onChange={onChange}
@@ -2402,6 +2403,7 @@ describe("SchemaForm multiline strings (#2042)", () => {
24022403
kind: { type: "string", const: value },
24032404
note: { type: "string", title: "Note" },
24042405
},
2406+
required: ["kind"],
24052407
});
24062408
const { rerender } = renderWithMantine(
24072409
<SchemaForm

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,24 @@ describe("resolveRootUnion", () => {
617617
});
618618
});
619619

620+
it("offers a branch that only adds required names over the root's fields", () => {
621+
// `anyOf: [{ required: ["email"] }, { required: ["phone"] }]` is an
622+
// ordinary way to say "one of these two". Judging the member on its own
623+
// properties declined it, leaving the gate checking the base alone and
624+
// accepting `{}` — which the schema rejects.
625+
const { branches } = resolveRootUnion({
626+
type: "object",
627+
properties: { email: { type: "string" }, phone: { type: "string" } },
628+
anyOf: [
629+
{ type: "object", required: ["email"] },
630+
{ type: "object", required: ["phone"] },
631+
],
632+
});
633+
expect(branches).toHaveLength(2);
634+
expect(branches[0].schema.required).toEqual(["email"]);
635+
expect(branches[1].schema.required).toEqual(["phone"]);
636+
});
637+
620638
describe("oneOf exclusivity", () => {
621639
it("offers a discriminated oneOf", () => {
622640
expect(

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -375,15 +375,32 @@ describe("root composition (#2123)", () => {
375375
expect(collectSchemaDefaults(UNION)).not.toHaveProperty("phone");
376376
});
377377

378-
it("seeds a const property on an ordinary schema too", () => {
378+
it("seeds a required const property on an ordinary schema too", () => {
379379
expect(
380380
collectSchemaDefaults({
381381
type: "object",
382382
properties: { version: { type: "string", const: "1" } },
383+
required: ["version"],
383384
}),
384385
).toEqual({ version: "1" });
385386
});
386387

388+
it("leaves an optional const out", () => {
389+
// `const` constrains a present value; it does not require the property or
390+
// act as a default, so seeding it would turn a valid `{}` call into one
391+
// that asks the server to do something.
392+
const optional: InspectorFormSchema = {
393+
type: "object",
394+
properties: { dryRun: { type: "boolean", const: true } },
395+
};
396+
expect(collectSchemaDefaults(optional)).toEqual({});
397+
expect(applySchemaConstants(optional, {})).toEqual({});
398+
// …but a value that IS present must be the one the schema fixes.
399+
expect(applySchemaConstants(optional, { dryRun: false })).toEqual({
400+
dryRun: true,
401+
});
402+
});
403+
387404
it("prefers a const over a conflicting default", () => {
388405
// `default` is an annotation, not a constraint, so a schema may advertise
389406
// one its own `const` rejects — seeding it would submit an invalid value
@@ -394,6 +411,7 @@ describe("root composition (#2123)", () => {
394411
properties: {
395412
v: { type: "string", const: "a", default: "b" },
396413
},
414+
required: ["v"],
397415
}),
398416
).toEqual({ v: "a" });
399417
});
@@ -454,6 +472,7 @@ describe("root composition (#2123)", () => {
454472
properties: Object.fromEntries([
455473
["__proto__", { type: "string", const: "kept" }],
456474
]),
475+
required: ["__proto__"],
457476
});
458477
expect(Object.hasOwn(seeded, "__proto__")).toBe(true);
459478
});
@@ -474,13 +493,15 @@ describe("root composition (#2123)", () => {
474493
kind: { type: "string", const: "email" },
475494
address: { type: "string" },
476495
},
496+
required: ["kind"],
477497
},
478498
{
479499
type: "object",
480500
properties: {
481501
kind: { type: "string", const: "sms" },
482502
phone: { type: "string" },
483503
},
504+
required: ["kind"],
484505
},
485506
],
486507
},

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,17 @@ export function collectSchemaDefaults(
145145
// to a form showing the branch the values actually identify.
146146
const { base, branches } = resolveRootUnion(schema);
147147
const selected = selectBranchIndex(branches, knownValues) ?? 0;
148-
const properties = (branches[selected]?.schema ?? base).properties ?? {};
148+
const effective = branches[selected]?.schema ?? base;
149+
const properties = effective.properties ?? {};
150+
// `const` constrains a property only when it is PRESENT — it neither requires
151+
// the property nor acts as a default. So it is supplied automatically for a
152+
// REQUIRED field, whose only submittable value was never in doubt and which
153+
// the form renders read-only, and left alone otherwise: seeding an optional
154+
// `dryRun: { const: true }` would turn a valid `{}` call into one that asks
155+
// the server to do something.
156+
const requiredNames = Array.isArray(effective.required)
157+
? effective.required
158+
: [];
149159
const result: Record<string, unknown> = {};
150160
// `properties` is a JSON record, so `__proto__` is a legal field name that a
151161
// plain assignment would drop into the legacy prototype setter rather than
@@ -164,10 +174,10 @@ export function collectSchemaDefaults(
164174
// without this the form would *display* a hoisted default that never
165175
// reached the seeded values — the field would submit empty (#1928).
166176
const fieldSchema = normalizeNullableUnion(rawSchema);
167-
if (fieldSchema.const !== undefined) {
168-
// `const` is a one-value enumeration, so the only submittable value is
169-
// already known — seeding it spares the user hand-typing a discriminator
170-
// the schema has fixed (#2123).
177+
if (fieldSchema.const !== undefined && requiredNames.includes(fieldName)) {
178+
// `const` is a one-value enumeration, so a required field's only
179+
// submittable value is already known — seeding it spares the user
180+
// hand-typing a discriminator the schema has fixed (#2123).
171181
//
172182
// It outranks `default`, which JSON Schema defines as an annotation
173183
// rather than a constraint: a schema may advertise a default its own
@@ -267,13 +277,21 @@ export function applySchemaConstants(
267277
): Record<string, unknown> {
268278
const { base, branches } = resolveRootUnion(schema);
269279
const selected = selectBranchIndex(branches, values) ?? 0;
270-
const properties = (branches[selected]?.schema ?? base).properties ?? {};
280+
const effective = branches[selected]?.schema ?? base;
281+
const properties = effective.properties ?? {};
282+
const required = Array.isArray(effective.required) ? effective.required : [];
271283

272284
const corrections: [string, unknown][] = [];
273285
for (const [name, rawSchema] of Object.entries(properties)) {
274286
const fieldSchema = normalizeNullableUnion(rawSchema);
275287
if (fieldSchema.const !== undefined) {
276-
corrections.push([name, fieldSchema.const]);
288+
// Corrected where the property is PRESENT — whatever supplied it, the
289+
// schema fixes the value — and inserted only where the schema also
290+
// requires it. An optional `const` the caller left out stays out:
291+
// `const` constrains a present value, it does not demand one.
292+
if (Object.hasOwn(values, name) || required.includes(name)) {
293+
corrections.push([name, fieldSchema.const]);
294+
}
277295
continue;
278296
}
279297
// Recurse: a nested object renders its own read-only fields, and the

‎core/json/rootUnion.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,23 @@ function propertiesOf(schema: RootUnionSchema): Record<string, unknown> {
175175
* `{ type: "string", properties: {…} }` member can never match — rendering it
176176
* as a fillable form would offer a call that cannot be valid.
177177
*/
178-
function isOfferable(branch: RootUnionSchema): boolean {
179-
if (!hasReadableProperties(branch) || !admitsObject(branch)) return false;
180-
const properties = Object.values(propertiesOf(branch));
178+
function isOfferable(
179+
branch: RootUnionSchema,
180+
merged: RootUnionSchema,
181+
): boolean {
182+
// The MEMBER decides whether the shape is readable at all…
183+
if (branch.properties !== undefined && !hasReadableProperties(branch)) {
184+
return false;
185+
}
186+
if (!admitsObject(branch)) return false;
187+
188+
// …and the MERGE decides whether there is anything to render. A member may
189+
// legitimately declare no properties of its own and only add `required` over
190+
// names the root already declares — `anyOf: [{ required: ["email"] }, …]` is
191+
// a perfectly ordinary way to say "one of these two" — and judging it on its
192+
// own properties would decline it, leaving the gate checking the base alone
193+
// and accepting `{}`, which the schema rejects.
194+
const properties = Object.values(propertiesOf(merged));
181195
return (
182196
properties.length > 0 &&
183197
// Every value has to be something a renderer can read AND something a
@@ -679,7 +693,7 @@ export function resolveRootUnion<T extends RootUnionSchema>(
679693
branches.some(
680694
(branch) =>
681695
branch === null ||
682-
!isOfferable(branch) ||
696+
!isOfferable(branch, mergeBranch(base, branch)) ||
683697
// The same faithfulness test the `allOf` fold applies: a member
684698
// carrying a constraint the merge does not copy — a nested `allOf`, a
685699
// `not`, a `$ref` — would have it erased along with the union keyword,

0 commit comments

Comments
 (0)