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

fix: address Copilot review rounds 7 and 8

Browse source 
- rootUnion: a `oneOf` demands that EXACTLY one alternative match, which
  flattening cannot preserve, so it is offered only when a discriminator makes
  the alternatives mutually exclusive; `anyOf` makes no such claim and is
  unaffected. An empty `additionalProperties` schema is the equivalent of
  `true` and no longer declines a union.
- selectBranchIndex: falls back to the branch whose own required fields the
  values supply, so an undiscriminated union's values open the picker on the
  shape they describe rather than on the first branch.
- admitsNull: honors a sibling `nullable: true` beside `const: null`.
- SchemaForm: a value is carried only where the incoming branch leaves the
  root's declaration as it found it; anything it declares itself is reset,
  since it may type the name differently from where the value was typed.
  `hasOwn`/`fromEntries` throughout, so `constructor` is not read as a
  supplied value and `__proto__` is not dropped into the prototype setter.
- TUI: the same prototype-safe construction on the way out of the form, and
  the chosen branch's `required` list is checked at submit — rendering a
  branch's fields optional makes the FORM satisfiable, not the call, so the
  missing names are reported rather than a known-invalid call being sent.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 03:54:24 -04:00
commit 85b8e4601c
4 changed files with 492 additions and 42 deletions
Download patch file Download diff file Expand all files Collapse all files

75
clients/tui/__tests__/ToolTestModal.test.tsx
View file

@ -74,6 +74,81 @@ const renderAndSubmit = async (
};
describe("ToolTestModal", () => {
it("reports a missing required argument instead of calling the tool (#2123)", async () => {
// A union branch's fields render optional — a static form cannot demand
// every branch's — so the chosen shape's own requirements are checked here.
const callTool = vi.fn();
const tool = makeTool({
inputSchema: {
type: "object",
oneOf: [
{
type: "object",
properties: {
kind: { type: "string", const: "email" },
address: { type: "string" },
},
required: ["kind", "address"],
},
{
type: "object",
properties: {
kind: { type: "string", const: "sms" },
phone: { type: "string" },
},
required: ["kind", "phone"],
},
],
},
} as unknown as Partial<Tool>);
const api = render(
<ToolTestModal
tool={tool}
inspectorClient={fakeClient(callTool)}
width={80}
height={24}
onClose={vi.fn()}
/>,
);
await tick();
setSubmitValue({ __variant: "0", __b0__kind: "email" });
api.stdin.write("\r");
await tick();
await tick();
// The assertion is the transition, not the frame — this suite drives state,
// not rendered text (see the note at the top of the file).
expect(callTool).not.toHaveBeenCalled();
api.unmount();
});
it("names every missing required argument (#2123)", async () => {
const callTool = vi.fn();
const tool = makeTool({
inputSchema: {
type: "object",
properties: { a: { type: "string" }, b: { type: "string" } },
required: ["a", "b"],
},
} as unknown as Partial<Tool>);
const api = render(
<ToolTestModal
tool={tool}
inspectorClient={fakeClient(callTool)}
width={80}
height={24}
onClose={vi.fn()}
/>,
);
await tick();
setSubmitValue({});
api.stdin.write("\r");
await tick();
await tick();
// Two missing names, which is also the plural branch of the message.
expect(callTool).not.toHaveBeenCalled();
api.unmount();
});
it("renders the form initially without invoking the client", async () => {
const callTool = vi.fn();
const api = render(

125
clients/tui/__tests__/schemaToForm.test.ts
View file

@ -1,5 +1,9 @@
import { describe, it, expect } from "vitest";
import { decodeFormValues, schemaToForm } from "../src/utils/schemaToForm.js";
import {
decodeFormValues,
missingRequiredFields,
schemaToForm,
} from "../src/utils/schemaToForm.js";
describe("schemaToForm", () => {
it("returns an empty Parameters section when there is no schema", () => {
@ -655,6 +659,26 @@ describe("schemaToForm", () => {
).toEqual({ n: 7, a: "x" });
});
it("keeps a decoded argument named __proto__", () => {
// Assigning it would invoke the legacy prototype setter, so a field
// prefixed safely in the form would vanish on the way to the call.
const schema = {
type: "object",
anyOf: [
{
type: "object",
properties: { ["__proto__"]: { type: "string" } },
},
{ type: "object", properties: { other: { type: "string" } } },
] as unknown[],
};
const decoded = decodeFormValues(schema, {
__variant: "0",
__b0____proto__: "kept",
});
expect(Object.hasOwn(decoded, "__proto__")).toBe(true);
});
it("keeps a base argument whose name looks generated", () => {
const schema = {
type: "object",
@ -695,4 +719,103 @@ describe("schemaToForm", () => {
expect(form.sections).toEqual([{ title: "Parameters", fields: [] }]);
});
});
describe("edge shapes (#2123)", () => {
it("renders a union that declares no root properties", () => {
const form = schemaToForm(
{
type: "object",
anyOf: [
{ type: "object", properties: { a: { type: "string" } } },
{ type: "object", properties: { b: { type: "string" } } },
],
},
"no_base",
);
expect(form.sections[0]!.fields.map((f) => f.name)).toEqual([
"__variant",
]);
expect(form.sections[1]!.fields.map((f) => f.name)).toEqual(["__b0__a"]);
});
it("returns the values unchanged for a schema with no properties", () => {
const values = { anything: "x" };
expect(decodeFormValues({ type: "object" }, values)).toBe(values);
});
it("ignores a malformed property declaration when restoring constants", () => {
// `properties` values are `unknown`; a `null` entry must not throw on the
// way out of the form any more than it does on the way in.
const schema = {
type: "object",
anyOf: [
{
type: "object",
properties: { broken: null, kind: { const: "a" } },
},
{ type: "object", properties: { kind: { const: "b" } } },
] as unknown[],
};
expect(
decodeFormValues(schema, { __variant: "0", __b0__kind: "tampered" }),
).toEqual({ kind: "a" });
});
});
describe("missingRequiredFields (#2123)", () => {
const REQUIRED_UNION = {
type: "object",
oneOf: [
{
type: "object",
properties: {
kind: { type: "string", const: "email" },
address: { type: "string" },
},
required: ["kind", "address"],
},
{
type: "object",
properties: {
kind: { type: "string", const: "sms" },
phone: { type: "string" },
},
required: ["kind", "phone"],
},
],
};
it("reports what the chosen branch requires and the values omit", () => {
// A branch's fields render optional — a static form cannot demand every
// branch's — so the requirement is checked against the chosen shape here
// rather than sending a call known to violate the schema.
expect(
missingRequiredFields(
REQUIRED_UNION,
{ kind: "email" },
{ __variant: "0" },
),
).toEqual(["address"]);
});
it("reports nothing once the chosen branch is satisfied", () => {
expect(
missingRequiredFields(
REQUIRED_UNION,
{ kind: "sms", phone: "555" },
{ __variant: "1" },
),
).toEqual([]);
});
it("checks the root's own required fields when there is no union", () => {
const schema = {
type: "object",
properties: { message: { type: "string" } },
required: ["message"],
};
expect(missingRequiredFields(schema, {})).toEqual(["message"]);
expect(missingRequiredFields(schema, { message: "hi" })).toEqual([]);
});
});
});

23
clients/tui/src/components/ToolTestModal.tsx
View file

@ -5,7 +5,11 @@ import { InspectorClient } from "@inspector/core/mcp/index.js";
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
import type { Tool, CallToolResult } from "@modelcontextprotocol/client";
import type { JsonValue } from "@inspector/core/mcp/index.js";
import { decodeFormValues, schemaToForm } from "../utils/schemaToForm.js";
import {
decodeFormValues,
missingRequiredFields,
schemaToForm,
} from "../utils/schemaToForm.js";
import { ScrollView, type ScrollViewRef } from "ink-scroll-view";
interface ToolTestModalProps {
@ -123,6 +127,23 @@ export function ToolTestModal({
// the base fields plus the chosen branch's, and nothing from the others.
const values = decodeFormValues(tool.inputSchema, rawValues);
// A branch's fields are rendered optional — only one alternative applies to
// a call, and requiring every branch's would deadlock a static form — so
// the chosen shape's own requirements are checked here instead. Reported
// rather than sent: a call known to violate the schema teaches the user
// nothing about the server (#2123).
const missing = missingRequiredFields(tool.inputSchema, values, rawValues);
if (missing.length > 0) {
setResult({
input: values,
output: null,
error: `Missing required argument${missing.length > 1 ? "s" : ""}: ${missing.join(", ")}`,
duration: 0,
});
setState("results");
return;
}
setState("loading");
const startTime = Date.now();

92
clients/tui/src/utils/schemaToForm.ts
View file

@ -138,7 +138,7 @@ function branchFields(
branches: { declaredFields: string[] }[],
index: number,
): string[] {
const own = branches[index]?.declaredFields ?? [];
const own = branches[index]!.declaredFields;
return [...new Set([...own, ...sharedFieldNames(base, branches)])];
}
@ -256,11 +256,7 @@ export function decodeFormValues<T>(
}
const { variant, prefix } = generatedNames(base, branches);
const selected = Number(values[variant]);
const branchIndex =
Number.isInteger(selected) && selected >= 0 && selected < branches.length
? selected
: 0;
const branchIndex = selectedBranchIndex(base, branches, values);
const branch = branches[branchIndex]!;
const generated = new Set<string>([variant]);
@ -270,21 +266,42 @@ export function decodeFormValues<T>(
}
});
const decoded: Record<string, T> = {};
for (const [name, value] of Object.entries(values)) {
if (!generated.has(name)) {
decoded[name] = value;
}
}
for (const name of branchFields(base, branches, branchIndex)) {
const value = values[branchFieldName(prefix, branchIndex, name)];
if (value !== undefined) {
decoded[name] = value;
}
}
// Built through `fromEntries` rather than by assignment: `__proto__` is a
// legal argument name, and assigning it would invoke the legacy prototype
// setter — the field would be prefixed safely in the form and then vanish on
// the way to the call.
const decoded: Record<string, T> = Object.fromEntries([
...Object.entries(values).filter(([name]) => !generated.has(name)),
...branchFields(base, branches, branchIndex)
.map(
(name) =>
[name, values[branchFieldName(prefix, branchIndex, name)]] as const,
)
.filter(([, value]) => value !== undefined),
]);
/* v8 ignore next -- an offerable branch always carries properties */
return applyConstants(branch.schema.properties ?? {}, decoded);
}
/**
* Which branch the variant select names, clamped to one that exists — a form
* value is whatever the user's terminal produced, and the fallback is the first
* branch, which is what the select opens on.
*/
function selectedBranchIndex(
base: { properties?: Record<string, unknown> },
branches: { declaredFields: string[] }[],
values: Record<string, unknown>,
): number {
const { variant } = generatedNames(base, branches);
const selected = Number(values[variant]);
return Number.isInteger(selected) &&
selected >= 0 &&
selected < branches.length
? selected
: 0;
}
/** Overwrite every `const`-pinned field with the value its schema fixes. */
function applyConstants<T>(
properties: Record<string, unknown>,
@ -294,11 +311,10 @@ function applyConstants<T>(
([, schema]) => constOf(schema) !== undefined,
);
if (pinned.length === 0) return values;
const result = { ...values };
for (const [name, schema] of pinned) {
result[name] = constOf(schema) as T;
}
return result;
return Object.fromEntries([
...Object.entries(values),
...pinned.map(([name, schema]) => [name, constOf(schema) as T]),
]);
}
/** Build the ink-form fields for one already-flattened object schema. */
@ -365,6 +381,7 @@ function buildFields(schema: JsonSchemaObject): FormField[] {
field = {
type: "select",
...baseField,
/* v8 ignore next -- guarded by `isStringEnum(property.enum)` above */
options: toSelectOptions(property.enum ?? [], property.enumNames),
} as FormField;
} else {
@ -420,3 +437,32 @@ function buildFields(schema: JsonSchemaObject): FormField[] {
return fields;
}
/**
* The required fields the chosen shape does not supply — what a static form
* cannot enforce for itself.
*
* A branch's fields are rendered optional because only one alternative applies
* to a call and ink-form would otherwise demand every branch's, deadlocking the
* form. That makes the *form* satisfiable, not the call: selecting `email` and
* leaving `address` empty still violates the schema. So the requirement is
* checked at submit instead, against the branch the variant select names, and
* the caller reports it rather than sending a call known to be invalid.
*
* Takes the **decoded** values — what would actually be sent.
*/
export function missingRequiredFields(
schema: JsonSchemaObject | null | undefined,
decoded: Record<string, unknown>,
rawValues: Record<string, unknown> = {},
): string[] {
const { base, branches } = resolveRootUnion(schema ?? {});
const effective =
branches.length === 0
? base
: branches[selectedBranchIndex(base, branches, rawValues)]!.schema;
return (effective.required ?? []).filter((name) => {
const value = decoded[name];
return value === undefined || value === "";
});
}