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

fix: make the collapse and null-admission fail closed on unevaluated schemas

Browse source 
Copilot round 8 opened the same conjunctive-semantics seam a third time, so
this stops patching cases and inverts the defaults instead. Both guards now
fail CLOSED: anything this module cannot evaluate sends the field to the JSON
editor rather than to a widget built from a partial reading.

1. The wrapper-conflict check enumerated the *validation* keywords and declined
   when two of them disagreed — which meant any keyword nobody listed
   (`oneOf`, `allOf`, `not`, `$ref`, a future vocabulary) was silently treated
   as safe to drop. That whitelist fails open, and open means rendering a
   widget for a constraint that is not the schema's. It is replaced by an
   allowlist of *annotation* keywords: the collapse declines whenever the
   wrapper carries anything beyond annotations and the union itself. Stricter
   than the old equality test — a wrapper restating its branch's `type` now
   declines too — which costs a dropdown on a rare redundant shape and buys a
   rule with no gap in it.

2. `admitsNull` returned true for shapes it had not actually evaluated:
   `not: { type: "null" }` rules null out, `allOf` can add a member that does,
   `oneOf` requires *exactly one* branch to match (so a null branch does not
   by itself mean null validates), and a branch like `{ type: "null",
   const: "x" }` is unsatisfiable. All now answer false. Under-claiming
   nullability costs a clear button; over-claiming lets the form emit a value
   the schema rejects, so the asymmetry decides it.

Note the third finding — draft validity never reaching the submit controls —
is NOT fixed here, and it is worse than I described last round: `ToolDetailPanel`
gates Execute on `isExecuting` alone and never calls `hasMissingRequiredFields`,
so my earlier claim that required fields were already covered was wrong for the
Tools tab. Corrected on #2020, which is where the fix belongs.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 19:44:09 -04:00
commit 2fdbd1905f
2 changed files with 131 additions and 77 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -418,21 +418,24 @@ describe("normalizeNullableUnion", () => {
expect(normalizeNullableUnion(schema)).toBe(schema);
});
// Identical values are no conflict, so the ordinary redundant-type shape
// still collapses rather than being punished for being explicit.
it("collapses when wrapper and branch agree on a shared keyword", () => {
expect(
normalizeNullableUnion({
type: "string",
anyOf: [{ type: "string", enum: ["a"] }, { type: "null" }],
}),
).toEqual({
// Deliberately stricter than value-equality: a wrapper restating its branch's
// `type` also declines. That costs a dropdown on a rare redundant shape and
// buys a rule with no gap, which is the better trade for a module that cannot
// evaluate JSON Schema.
it("declines even when wrapper and branch agree on a shared keyword", () => {
const schema = {
type: "string",
enum: ["a"],
anyOf: undefined,
// The explicit non-null `type` rules null out, conjunctively.
nullable: false,
});
anyOf: [{ type: "string", enum: ["a"] }, { type: "null" as const }],
};
expect(normalizeNullableUnion(schema)).toBe(schema);
});
it("declines when the wrapper carries an applicator the module cannot read", () => {
const schema = {
allOf: [{ minLength: 2 }],
anyOf: [{ type: "string" }, { type: "null" as const }],
};
expect(normalizeNullableUnion(schema)).toBe(schema);
});
it("treats wrapper metadata as safe to keep alongside branch constraints", () => {
@ -484,13 +487,51 @@ describe("admitsNull", () => {
expect(admitsNull(wide)).toBe(true);
});
it("recognizes a null branch in a oneOf, and a nested type array", () => {
expect(admitsNull({ oneOf: [{ type: "string" }, { type: "null" }] })).toBe(
true,
);
it("recognizes a null branch declared as a nested type array", () => {
expect(admitsNull({ anyOf: [{ type: ["string", "null"] }] })).toBe(true);
});
// Applicators this module does not evaluate. `oneOf` requires *exactly one*
// branch to match, so a null branch does not by itself mean null validates;
// `not` and `allOf` can rule it out outright. Under-claiming costs a clear
// button, over-claiming lets the form emit a value the schema rejects.
it("declines to claim nullability for applicators it cannot evaluate", () => {
expect(admitsNull({ oneOf: [{ type: "string" }, { type: "null" }] })).toBe(
false,
);
expect(
admitsNull({ not: { type: "null" }, anyOf: [{ type: "null" }] }),
).toBe(false);
expect(
admitsNull({ allOf: [{ type: "string" }], anyOf: [{ type: "null" }] }),
).toBe(false);
});
// A branch can name null and still admit nothing.
it("ignores an unsatisfiable null branch", () => {
expect(
admitsNull({ anyOf: [{ type: "string" }, { type: "null", const: "x" }] }),
).toBe(false);
expect(
admitsNull({
anyOf: [{ type: "string" }, { type: "null", enum: ["x"] }],
}),
).toBe(false);
expect(
admitsNull({
anyOf: [{ type: "string" }, { type: "null", not: { type: "null" } }],
}),
).toBe(false);
});
it("still accepts a plain null branch alongside an unsatisfiable one", () => {
expect(
admitsNull({
anyOf: [{ type: "null", const: "x" }, { type: "null" }],
}),
).toBe(true);
});
it("is false for a schema that does not permit null", () => {
expect(admitsNull({ type: "string" })).toBe(false);
expect(admitsNull({ type: ["string", "number"] })).toBe(false);

133
core/json/nullableUnion.ts
View file

@ -45,6 +45,8 @@ export interface NullableUnionSchema {
// Read only by `admitsNull`, which considers encodings the collapse itself
// declines to flatten, and the sibling constraints that can rule null out.
oneOf?: readonly unknown[];
allOf?: readonly unknown[];
not?: unknown;
nullable?: boolean;
const?: unknown;
}
@ -127,69 +129,63 @@ function nullExcludedBySiblings(schema: NullableUnionSchema): boolean {
}
/**
* Keywords that *constrain what values are valid*, as opposed to describing the
* field (`title`, `description`, `default`, `enumNames`).
* Keywords that only *describe* a field. Everything else constrains what values
* are valid.
*
* Only these can conflict in a way that matters: a wrapper and its branch each
* carrying one means the value must satisfy **both**, which the hoist cannot
* express. Metadata is safe to merge because nothing validates against it.
* This is an **allowlist on purpose**, and the direction matters. The previous
* version enumerated the *validation* keywords and declined on a conflict among
* them, which meant any keyword nobody thought of — `oneOf`, `allOf`, `not`,
* `$ref`, a future vocabulary — was silently treated as safe to drop. A missing
* entry there fails open, and open means rendering a widget for a constraint
* that is not the schema's. Listing the harmless keys instead fails closed: an
* unrecognized keyword sends the field to the JSON editor, which is the correct
* answer for a schema this module does not understand.
*/
const VALIDATION_KEYWORDS = [
"type",
"enum",
"const",
"items",
"properties",
"required",
"additionalProperties",
"minimum",
"maximum",
"exclusiveMinimum",
"exclusiveMaximum",
"multipleOf",
"minLength",
"maxLength",
"pattern",
"format",
"minItems",
"maxItems",
"uniqueItems",
] as const;
const ANNOTATION_KEYWORDS = new Set([
"title",
"description",
"default",
"enumNames",
"examples",
"deprecated",
"readOnly",
"writeOnly",
"$comment",
"$schema",
"$id",
]);
/**
* Whether hoisting this branch would *drop* one of the wrapper's constraints.
* Whether hoisting a branch would *drop* something the wrapper constrains.
*
* The hoist is a spread, so a branch key replaces the wrapper's — but JSON
* Schema applies sibling keywords **conjunctively**, and a replacement is not a
* conjunction. A wrapper `enum: ["a"]` around a branch `enum: ["a", "b"]` means
* `"a"` (both must hold), yet the spread yields `["a", "b"]` and the rendered
* dropdown would offer — and submit — a `"b"` the schema rejects. `type`,
* bounds, and the object keywords fail the same way.
* bounds, the object keywords, and the applicators (`oneOf`, `allOf`, `not`)
* all fail the same way.
*
* Rather than implement intersection for every keyword (a much larger surface,
* and one where a subtly wrong intersection is worse than none), the collapse
* **declines** when wrapper and branch disagree, and the field renders through
* the JSON editor with its full schema intact. Identical values are not a
* conflict, so the common `{ type: "string", anyOf: [{ type: "string", … }] }`
* still collapses.
* Rather than intersect them — a much larger surface, and one where a subtly
* wrong intersection is worse than none because it renders a *plausible* widget
* for the wrong constraint — the collapse **declines** whenever the wrapper
* carries anything beyond annotations and the union itself. The field then
* renders through the JSON editor with its full schema intact.
*
* Comparison is by canonical JSON, so two structurally equal objects written
* with different key order read as a conflict. That is conservative in the safe
* direction: the field falls back rather than mis-renders.
* This is stricter than comparing values for equality: a wrapper that merely
* restates its branch's `type` now also declines. That costs a dropdown on a
* rare redundant shape and buys a rule with no gap in it, which is the better
* trade for a module that cannot evaluate JSON Schema.
*/
function branchDropsWrapperConstraint(
schema: NullableUnionSchema,
branch: Record<string, unknown>,
): boolean {
const wrapper = schema as Record<string, unknown>;
return VALIDATION_KEYWORDS.some((keyword) => {
const wrapperValue = wrapper[keyword];
const branchValue = branch[keyword];
if (wrapperValue === undefined || branchValue === undefined) {
return false;
}
return JSON.stringify(wrapperValue) !== JSON.stringify(branchValue);
});
function wrapperCarriesConstraints(schema: NullableUnionSchema): boolean {
return Object.keys(schema).some(
(key) => key !== "anyOf" && !ANNOTATION_KEYWORDS.has(key),
);
}
/** Whether a schema's `type` names `"null"` at all. */
function typeNamesNull(type: unknown): boolean {
return Array.isArray(type) ? type.includes("null") : type === "null";
}
/**
@ -313,6 +309,19 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
if (nullExcludedBySiblings(schema)) {
return false;
}
// Applicators this module does not evaluate. `not: { type: "null" }` rules
// null out; `allOf` can add a member that does; and `oneOf` requires
// **exactly one** branch to match, so a null branch does not by itself mean
// `null` validates — two matching branches make it fail. Rather than answer
// these half-correctly, say no: under-claiming nullability costs a clear
// button, while over-claiming lets the form emit a value the schema rejects.
if (
schema.not !== undefined ||
schema.allOf !== undefined ||
schema.oneOf !== undefined
) {
return false;
}
if (schema.nullable === true) {
return true;
}
@ -322,21 +331,25 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
// as the union. Falling through to the branch scan here would let the union
// override a constraint that outranks it.
if (schema.type !== undefined) {
return Array.isArray(schema.type)
? schema.type.includes("null")
: schema.type === "null";
return typeNamesNull(schema.type);
}
return [schema.anyOf, schema.oneOf].some((branches) =>
branches?.some((entry) => {
return (
schema.anyOf?.some((entry) => {
const branch = toBranch(entry);
if (branch === null) {
if (branch === null || !typeNamesNull(branch.type)) {
return false;
}
// A branch that names null can still admit nothing: `{ type: "null",
// const: "x" }` is unsatisfiable, and its own applicators are as opaque
// here as the wrapper's.
const branchSchema = branch as NullableUnionSchema;
return (
branch.type === "null" ||
(Array.isArray(branch.type) && branch.type.includes("null"))
!nullExcludedBySiblings(branchSchema) &&
branchSchema.not === undefined &&
branchSchema.allOf === undefined &&
branchSchema.oneOf === undefined
);
}),
}) ?? false
);
}
@ -375,7 +388,7 @@ export function normalizeNullableUnion<T extends NullableUnionSchema>(
// Hoisting would silently drop a wrapper constraint the branch also
// carries, so decline instead; see `branchDropsWrapperConstraint`.
if (nullBranch && branch && !branchDropsWrapperConstraint(schema, branch)) {
if (nullBranch && branch && !wrapperCarriesConstraints(schema)) {
// A branch may carry an `enum` and no `type`; JSON Schema allows that, and
// an all-string enum is unambiguously a string field. See isStringEnum for
// why a non-string enum deliberately does not get the same treatment.