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

fix: apply the fail-closed rule uniformly, and correct the admitsNull docs

Browse source 
Copilot round 9: four findings, all of them places where round 8's fail-closed
inversion was the right idea applied incompletely. The rule was only enforced
at the positions I happened to be editing; it now holds everywhere a schema is
inspected.

1. `admitsNull` returned early on an explicit `type`, ignoring a sibling
   `anyOf` — but the two are conjunctive, so `{ type: ["string", "null"],
   anyOf: [{ type: "string" }] }` admits no null at all. A sibling union now
   withholds the claim.

2. A null-typed branch could be unsatisfiable via its own nested `anyOf`
   (`{ type: "null", anyOf: [{ type: "string" }] }`). The branch check tested
   `not`/`allOf`/`oneOf` but not `anyOf`; it now treats a nested union as
   equally opaque.

3. The `type: [T, "null"]` collapse path had no applicator guard at all —
   `collapsed` clears `anyOf` unconditionally and the renderers ignore the
   rest, so `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` was widened
   into an unconstrained string field. That path now declines the same way the
   `anyOf` path does.

4. The `admitsNull` docstring still advertised `oneOf` support that round 8
   deliberately removed. Rewritten to state what is recognized, what is
   declined and why, and the asymmetry that settles the direction.

The three code fixes share one helper (`hasOpaqueApplicator`) rather than
repeating the condition, so the next position that needs it cannot drift.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 19:58:07 -04:00
commit 274a328abe
2 changed files with 97 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -357,6 +357,26 @@ describe("normalizeNullableUnion", () => {
});
});
// 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.
it("declines a type-array nullable that also carries a sibling anyOf", () => {
const schema = {
type: ["string", "null"],
anyOf: [{ const: "a" }],
};
expect(normalizeNullableUnion(schema)).toBe(schema);
});
it("declines a type-array nullable that carries an opaque applicator", () => {
const withNot = { type: ["string", "null"], not: { const: "a" } };
expect(normalizeNullableUnion(withNot)).toBe(withNot);
const withOneOf = { type: ["string", "null"], oneOf: [{ const: "a" }] };
expect(normalizeNullableUnion(withOneOf)).toBe(withOneOf);
const withAllOf = { type: ["string", "null"], allOf: [{ minLength: 2 }] };
expect(normalizeNullableUnion(withAllOf)).toBe(withAllOf);
});
it("normalizes type array array|null", () => {
expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({
type: "array",
@ -524,6 +544,17 @@ describe("admitsNull", () => {
).toBe(false);
});
it("ignores a null branch made unsatisfiable by its own nested anyOf", () => {
expect(
admitsNull({
anyOf: [
{ type: "string" },
{ type: "null", anyOf: [{ type: "string" }] },
],
}),
).toBe(false);
});
it("still accepts a plain null branch alongside an unsatisfiable one", () => {
expect(
admitsNull({
@ -588,14 +619,20 @@ describe("admitsNull", () => {
});
it("still honors an explicit type that does admit null", () => {
expect(admitsNull({ type: ["string", "null"] })).toBe(true);
expect(admitsNull({ type: "null" })).toBe(true);
});
// A sibling union is conjunctive with the `type`, so it can reject null even
// when the type list names it. It is not evaluated here, so its mere presence
// withholds the claim.
it("withholds the claim when a sibling anyOf sits beside an explicit type", () => {
expect(
admitsNull({
type: ["string", "null"],
anyOf: [{ type: "string" }, { type: "null" }],
}),
).toBe(true);
admitsNull({ type: ["string", "null"], anyOf: [{ type: "string" }] }),
).toBe(false);
// Unsatisfiable: names null, admits nothing.
expect(admitsNull({ type: "null", anyOf: [{ type: "string" }] })).toBe(
true,
false,
);
});

66
core/json/nullableUnion.ts
View file

@ -188,6 +188,23 @@ function typeNamesNull(type: unknown): boolean {
return Array.isArray(type) ? type.includes("null") : type === "null";
}
/**
* Applicators this module does not evaluate, and which are **conjunctive** with
* everything beside them — so any of them can rule `null` out, or constrain a
* value in a way no widget here would honor.
*
* `anyOf` is excluded because it is the keyword being *recognized*: a caller
* that has already identified the nullable-union shape checks it directly. Every
* other position treats a stray `anyOf` as opaque too.
*/
function hasOpaqueApplicator(schema: NullableUnionSchema): boolean {
return (
schema.not !== undefined ||
schema.allOf !== undefined ||
schema.oneOf !== undefined
);
}
/**
* What {@link stripNullEnumMembers} concluded about an `enum`.
*
@ -301,9 +318,24 @@ function collapsed<T extends NullableUnionSchema>(
* other would make a form reject a value its own schema accepts, which is why
* these are two functions rather than a `.nullable` flag read off the collapse.
*
* Recognizes every encoding the collapse does, plus the ones it declines:
* `nullable: true`, `type: "null"`, `type: [..., "null"]`, and a `"null"`
* branch anywhere in an `anyOf` or `oneOf` of any size.
* **Answers `false` for anything it cannot fully evaluate**, which is a larger
* set than it may look. Recognized: `nullable: true`, `type: "null"`,
* `type: [..., "null"]`, and a `"null"` branch in an `anyOf` of any size.
* Deliberately *not* recognized, despite naming null:
*
* - **`oneOf`** — it requires *exactly one* branch to match, so a `null` branch
* does not mean `null` validates; two matching branches make it fail.
* - **`not` / `allOf`**, at this level or on the branch, which can rule null out.
* - **A sibling `anyOf` beside an explicit `type`**, since the two are
* conjunctive and the union is not evaluated here.
* - **A branch that names null but is unsatisfiable**, whether by its own
* `enum`/`const` or its own nested applicators.
*
* The asymmetry is what settles the direction: under-claiming costs a clear
* button and treats a valid `null` as missing, while over-claiming lets the
* form emit a value the schema rejects. So callers get a firm "no" rather than
* an optimistic guess, and a schema this module cannot read renders through the
* JSON editor with its constraints intact.
*/
export function admitsNull(schema: NullableUnionSchema): boolean {
if (nullExcludedBySiblings(schema)) {
@ -315,11 +347,7 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
// `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
) {
if (hasOpaqueApplicator(schema)) {
return false;
}
if (schema.nullable === true) {
@ -331,6 +359,13 @@ 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) {
// A sibling `anyOf` is conjunctive with the `type`, so it can reject null
// even when the type list names it — `{ type: ["string", "null"], anyOf:
// [{ type: "string" }] }` admits no null at all. Not evaluated here, so
// its mere presence withholds the claim.
if (schema.anyOf !== undefined) {
return false;
}
return typeNamesNull(schema.type);
}
return (
@ -345,9 +380,10 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
const branchSchema = branch as NullableUnionSchema;
return (
!nullExcludedBySiblings(branchSchema) &&
branchSchema.not === undefined &&
branchSchema.allOf === undefined &&
branchSchema.oneOf === undefined
!hasOpaqueApplicator(branchSchema) &&
// A nested union inside the branch is as opaque as the others: `{ type:
// "null", anyOf: [{ type: "string" }] }` names null and admits nothing.
branchSchema.anyOf === undefined
);
}) ?? false
);
@ -408,7 +444,13 @@ export function normalizeNullableUnion<T extends NullableUnionSchema>(
if (
Array.isArray(schema.type) &&
schema.type.length === 2 &&
schema.type.includes("null")
schema.type.includes("null") &&
// This path keeps the schema's own keywords, but `collapsed` clears `anyOf`
// and the renderers ignore the other applicators — so a compound schema
// like `{ type: ["string", "null"], anyOf: [{ const: "a" }] }` would be
// widened into an unconstrained string field. Leave it for the JSON editor.
schema.anyOf === undefined &&
!hasOpaqueApplicator(schema)
) {
const type = schema.type.find((member) => member !== "null");
if (isRenderableType(type)) {