modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: decline empty enums, drop mislabelling enumNames, treat $ref as opaque

Browse source 
Copilot round 11. Two of the three are the enum-strip logic this PR added
misbehaving on degenerate input; the third is one more keyword for the
fail-closed rule.

1. An empty `enum` permits nothing at all, but the strip reported "unchanged"
   (nothing was removed) and the collapse built a Select with no options — a
   required field then had no interaction able to supply a value. It now
   reaches the same conclusion as `[null]`: no selectable member, so decline.
   The outcome is renamed `unselectable`, since "only-null" no longer describes
   both routes to it.

2. Filtering `null` out of `enum` shortens it, which could make an ALREADY
   mismatched `enumNames` come out the same length — so `enum: [null, "b"]`
   with `enumNames: ["None"]` started labelling `b` as "None", turning labels
   both renderers had correctly ignored into a confident mislabel. A list that
   was not parallel before filtering is now dropped rather than kept.

3. `$ref` joins the opaque set. It is never resolved here and applies alongside
   its siblings, so `{ type: "string", $ref: "#/$defs/intOnly" }` can be
   unsatisfiable. The wrapper path already declined on it by virtue of the
   annotation allowlist; this extends the same rule to branches, the
   `type: [T, "null"]` path, and `admitsNull`.

No round 12 was requested: the review loop was stopped at the user's direction.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 20:27:19 -04:00
commit 18053d4cb2
2 changed files with 85 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

55
clients/web/src/test/core/nullableUnion.test.ts
View file

@ -341,7 +341,27 @@ describe("normalizeNullableUnion", () => {
});
});
it("leaves a mismatched enumNames alone rather than inventing an alignment", () => {
// Keeping a mismatched list is not neutral: filtering shortens `enum`, so a
// list both renderers had correctly *ignored* can come out the same length
// and start labelling — `enum: [null, "b"]` + `enumNames: ["None"]` would
// otherwise label `b` as "None". Dropping it preserves the ignore.
it("drops a mismatched enumNames rather than letting filtering align it", () => {
expect(
normalizeNullableUnion({
type: ["string", "null"],
enum: [null, "b"],
enumNames: ["None"],
}),
).toEqual({
type: "string",
enum: ["b"],
enumNames: undefined,
anyOf: undefined,
nullable: true,
});
});
it("drops a mismatched enumNames that stays mismatched too", () => {
expect(
normalizeNullableUnion({
type: ["string", "null"],
@ -351,12 +371,43 @@ describe("normalizeNullableUnion", () => {
).toEqual({
type: "string",
enum: ["a"],
enumNames: ["Alpha"],
enumNames: undefined,
anyOf: undefined,
nullable: true,
});
});
// An empty enum permits nothing at all, so it reaches the same conclusion as
// `[null]`: no value a dropdown could offer, so do not build one.
it("declines to collapse an empty enum", () => {
const typeArray = { type: ["string", "null"], enum: [] };
expect(normalizeNullableUnion(typeArray)).toBe(typeArray);
const union = {
anyOf: [{ type: "string", enum: [] }, { type: "null" as const }],
};
expect(normalizeNullableUnion(union)).toBe(union);
});
// `$ref` applies alongside its siblings and is never resolved here, so a
// branch carrying one can be unsatisfiable.
it("treats $ref as opaque wherever it appears", () => {
const onBranch = {
anyOf: [
{ type: "string", $ref: "#/$defs/intOnly" },
{ type: "null" as const },
],
};
expect(normalizeNullableUnion(onBranch)).toBe(onBranch);
const onTypeArray = { type: ["string", "null"], $ref: "#/$defs/intOnly" };
expect(normalizeNullableUnion(onTypeArray)).toBe(onTypeArray);
expect(admitsNull({ type: ["string", "null"], $ref: "#/$defs/x" })).toBe(
false,
);
expect(admitsNull({ anyOf: [{ type: "null", $ref: "#/$defs/x" }] })).toBe(
false,
);
});
// The type-array path keeps the schema's own keywords, but `collapsed` clears
// `anyOf` and the renderers ignore the other applicators — so a compound
// schema would be widened into an unconstrained field rather than collapsed.

53
core/json/nullableUnion.ts
View file

@ -47,6 +47,7 @@ export interface NullableUnionSchema {
oneOf?: readonly unknown[];
allOf?: readonly unknown[];
not?: unknown;
$ref?: string;
nullable?: boolean;
const?: unknown;
}
@ -201,7 +202,12 @@ function hasOpaqueApplicator(schema: NullableUnionSchema): boolean {
return (
schema.not !== undefined ||
schema.allOf !== undefined ||
schema.oneOf !== undefined
schema.oneOf !== undefined ||
// `$ref` points at a schema this module never resolves, and it applies
// *alongside* its siblings — `{ type: "string", $ref: "#/$defs/intOnly" }`
// can be unsatisfiable. The wrapper path already declines on it (it is not
// an annotation); this is the same rule for every other position.
schema.$ref !== undefined
);
}
@ -228,13 +234,13 @@ function hasUnevaluatedComposition(schema: NullableUnionSchema): boolean {
/**
* What {@link stripNullEnumMembers} concluded about an `enum`.
*
* `"only-null"` is a distinct outcome rather than "an empty list" because it
* `"unselectable"` is a distinct outcome rather than "an empty list" because it
* has to **stop the collapse**, not just empty a field. See below.
*/
type EnumStrip =
| { kind: "unchanged" }
| { kind: "filtered"; members: unknown[]; names?: unknown }
| { kind: "only-null" };
| { kind: "unselectable" };
/**
* Drop `null` members from an `enum`, keeping the parallel `enumNames` aligned.
@ -252,12 +258,13 @@ type EnumStrip =
* the two lengths disagree — so stripping one without the other silently loses
* every label rather than just the dropped one's.
*
* **An enum of nothing but `null` reports `"only-null"` so the caller declines
* to collapse at all.** Emitting `enum: undefined` instead would turn a schema
* permitting *only* `null` into a plain string field that accepts arbitrary
* text — trading a cosmetic problem for a correctness one, since the form would
* then invite values the schema forbids. Left uncollapsed it renders through
* the JSON editor, which represents it honestly.
* **An enum that offers no selectable value reports `"unselectable"`, so the
* caller declines to collapse at all.** That covers `[null]` and the empty
* `[]`: the first permits only `null`, the second permits nothing. Emitting
* `enum: undefined` instead would turn either into a plain string field
* accepting arbitrary text — trading a cosmetic problem for a correctness one,
* since the form would then invite values the schema forbids. Left uncollapsed
* they render through the JSON editor, which represents them honestly.
*/
function stripNullEnumMembers(members: unknown, names: unknown): EnumStrip {
if (!Array.isArray(members)) {
@ -266,19 +273,23 @@ function stripNullEnumMembers(members: unknown, names: unknown): EnumStrip {
const kept = members
.map((member, index) => ({ member, index }))
.filter((entry) => entry.member !== null);
// An empty `enum` permits nothing at all, so it reaches the same conclusion
// as `[null]` by a different route: there is no value a dropdown could offer.
if (kept.length === 0) {
return { kind: "unselectable" };
}
if (kept.length === members.length) {
return { kind: "unchanged" };
}
if (kept.length === 0) {
return { kind: "only-null" };
}
// Only realign names that were positionally parallel to begin with; a
// mismatched list is already ignored by both renderers, so re-indexing it
// would invent an alignment the server never declared.
const aligned =
Array.isArray(names) && names.length === members.length
? kept.map((entry) => names[entry.index])
: names;
// Only realign names that were positionally parallel to begin with, and DROP
// them otherwise. Keeping a mismatched list is not neutral: filtering shortens
// `enum`, so `enum: [null, "b"]` with `enumNames: ["None"]` would come out the
// same length and start *labelling* `b` as "None" — turning labels both
// renderers had correctly ignored into a confident mislabel.
const wasParallel = Array.isArray(names) && names.length === members.length;
const aligned = wasParallel
? kept.map((entry) => names[entry.index])
: undefined;
return {
kind: "filtered",
members: kept.map((entry) => entry.member),
@ -309,7 +320,7 @@ function collapsed<T extends NullableUnionSchema>(
merged.enum,
(merged as { enumNames?: unknown }).enumNames,
);
if (strip.kind === "only-null") {
if (strip.kind === "unselectable") {
return null;
}
return {
@ -458,7 +469,7 @@ export function normalizeNullableUnion<T extends NullableUnionSchema>(
const type =
branch.type ?? (isStringEnum(branch.enum) ? "string" : undefined);
if (isRenderableType(type)) {
// `null` when the merged enum permits only `null`, which is not
// `null` when the merged enum offers nothing selectable, which is not
// collapsible — fall through and return the schema by identity.
const result = collapsed(schema, branch, type);
if (result !== null) {