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 round 3 — conjunctive keywords, unflattenable allOf, branch inference

Browse source 
- rootUnion: a branch restating a constraint the root states differently is
  declined (keywords at one level are conjunctive and no intersection is
  computed here), while annotations may disagree freely. An `allOf` with a
  member it cannot fold in — JSON Schema's boolean form, or a $ref whose
  constraints are unknown rather than absent — is left intact instead of
  merged-and-stripped, which would turn an unsatisfiable schema into a
  fillable form.
- rootUnion: `declaresAnyFields` counts the raw composition members, so
  `hasInputFields` reports fields for a union the resolver declines and an
  App tool is never auto-invoked with empty arguments.
- SchemaForm: the picker opens on the branch the supplied values identify
  (an App deep link overlays its args, so they can name another branch), and
  a branch switch no longer carries a value the outgoing branch declared —
  the branches may type the same name differently.
- TUI: a base property some branch redeclares is offered in EVERY branch's
  section under that branch's inherited or specialized declaration, so a
  branch that does not redeclare it can still supply a required root
  argument.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-26 02:01:44 -04:00
commit 1a271e4931
7 changed files with 442 additions and 69 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -536,6 +536,32 @@ describe("schemaToForm", () => {
});
});
it("offers a shared base property in every branch's section", () => {
const schema = {
type: "object",
properties: { count: {} },
required: ["count"],
anyOf: [
{ type: "object", properties: { count: { type: "integer" } } },
{ type: "object", properties: { other: { type: "string" } } },
],
};
const form = schemaToForm(schema, "shared");
// Rendered only in branch A's section, branch B could never supply the
// required root argument — the chosen branch's fields are what decode.
expect(form.sections[2]!.fields.map((field) => field.name)).toEqual([
"__b1__other",
"__b1__count",
]);
expect(
decodeFormValues(schema, {
__variant: "1",
__b1__other: "x",
__b1__count: "4",
}),
).toEqual({ other: "x", count: "4" });
});
it("keeps its generated names clear of the schema's own", () => {
const form = schemaToForm(
{

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

@ -116,6 +116,32 @@ function branchFieldName(
return `${prefix}${branchIndex}__${name}`;
}
/**
* Base-declared names that at least one branch also declares. They move out of
* the base section and into every branch's, so the branch showing is the one
* whose declaration renders — and so a branch that does not specialize the name
* still offers it rather than losing a root argument it must supply.
*/
function sharedFieldNames(
base: { properties?: Record<string, unknown> },
branches: { declaredFields: string[] }[],
): string[] {
const declared = new Set(branches.flatMap((branch) => branch.declaredFields));
return Object.keys(base.properties ?? {}).filter((name) =>
declared.has(name),
);
}
/** The property names one branch's section renders, under prefixed names. */
function branchFields(
base: { properties?: Record<string, unknown> },
branches: { declaredFields: string[] }[],
index: number,
): string[] {
const own = branches[index]?.declaredFields ?? [];
return [...new Set([...own, ...sharedFieldNames(base, branches)])];
}
/** The `const` a property schema pins its value to, if any. */
function constOf(schema: unknown): unknown {
if (typeof schema !== "object" || schema === null) return undefined;
@ -148,17 +174,18 @@ export function schemaToForm(
}
const { variant, prefix } = generatedNames(base, branches);
const branchDeclared = new Set(
branches.flatMap((branch) => branch.declaredFields),
);
const shared = sharedFieldNames(base, branches);
// The base section renders what the base *alone* declares. A property a
// branch also declares is rendered in that branch's section instead, showing
// the branch's specialization — root `count: {}` under branch
// `count: { type: "number" }` is a number field there, not a string here.
// The base section renders what the base *alone* declares. A property some
// branch also declares moves into every branch's section, so the branch's
// specialization is what renders there — root `count: {}` under branch
// `count: { type: "number" }` is a number field, not a string — while a
// branch that does not specialize it still offers it, under its own inherited
// declaration. Rendering it once in the base section instead would strand it:
// the chosen branch's decoded fields are what reach the call.
const baseProperties = Object.fromEntries(
Object.entries(base.properties ?? {}).filter(
([name]) => !branchDeclared.has(name),
([name]) => !shared.includes(name),
),
);
@ -187,7 +214,7 @@ export function schemaToForm(
// a form that can never be submitted.
branches.forEach((branch, index) => {
const properties = Object.fromEntries(
branch.declaredFields.map((name) => [
branchFields(base, branches, index).map((name) => [
branchFieldName(prefix, index, name),
branch.schema.properties?.[name],
]),
@ -237,8 +264,8 @@ export function decodeFormValues<T>(
const branch = branches[branchIndex]!;
const generated = new Set<string>([variant]);
branches.forEach((each, index) => {
for (const name of each.declaredFields) {
branches.forEach((_each, index) => {
for (const name of branchFields(base, branches, index)) {
generated.add(branchFieldName(prefix, index, name));
}
});
@ -249,7 +276,7 @@ export function decodeFormValues<T>(
decoded[name] = value;
}
}
for (const name of branch.declaredFields) {
for (const name of branchFields(base, branches, branchIndex)) {
const value = values[branchFieldName(prefix, branchIndex, name)];
if (value !== undefined) {
decoded[name] = value;

49
clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx
View file

@ -2100,6 +2100,55 @@ describe("SchemaForm multiline strings (#2042)", () => {
expect(mode.value).toBe("fast");
});
it("opens on the branch the supplied values identify", () => {
// A deep link overlays its args on the initial defaults, so values for
// one branch can arrive while the picker would otherwise open on another.
renderWithMantine(
<SchemaForm
schema={UNION_SCHEMA}
values={{ kind: "sms", phone: "555" }}
onChange={vi.fn()}
/>,
);
expect(
(screen.getByRole("textbox", { name: /Variant/ }) as HTMLInputElement)
.value,
).toBe("sms");
expect(screen.getByRole("textbox", { name: /Phone/ })).toBeTruthy();
});
it("does not carry a value the outgoing branch declared", async () => {
const user = userEvent.setup();
const onChange = vi.fn();
const schema: InspectorFormSchema = {
type: "object",
anyOf: [
{
type: "object",
title: "A",
properties: { value: { type: "number", title: "Value" } },
},
{
type: "object",
title: "B",
properties: { value: { type: "boolean", title: "Value" } },
},
],
};
renderWithMantine(
<SchemaForm
schema={schema}
values={{ value: 3 }}
onChange={onChange}
/>,
);
await user.click(screen.getByRole("textbox", { name: /Variant/ }));
await user.click(screen.getByRole("option", { name: "B" }));
// Branch B types `value` as a boolean; carrying the 3 would check the box
// and submit a number the branch rejects.
expect(onChange).toHaveBeenCalledWith({});
});
it("renders no picker for a single-branch union but still shows its fields", () => {
const schema: InspectorFormSchema = {
type: "object",

58
clients/web/src/components/groups/SchemaForm/SchemaForm.tsx
View file

@ -571,6 +571,36 @@ function resolveValue(
return getDefaultValue(fieldSchema);
}
/**
* The branch a set of values already identifies, or `null` when they identify
* none uniquely.
*
* A discriminated union pins its discriminator with `const`, so values carrying
* one name the branch they belong to. This matters because the form is not
* always mounted empty: an App deep link overlays `appArgs` on the initial
* defaults, so `{ kind: "sms", … }` can arrive while the picker would otherwise
* open on the first branch — showing one shape's controls while a differently
* shaped set of values sits underneath, ready to be submitted (#2123).
*/
function matchBranchIndex(
branches: { schema: InspectorFormSchema }[],
values: Record<string, unknown>,
): number | null {
const matches: number[] = [];
branches.forEach((branch, index) => {
const pinned = Object.entries(branch.schema.properties ?? {}).filter(
([, fieldSchema]) => fieldSchema.const !== undefined,
);
if (
pinned.length > 0 &&
pinned.every(([name, fieldSchema]) => values[name] === fieldSchema.const)
) {
matches.push(index);
}
});
return matches.length === 1 ? matches[0] : null;
}
export function SchemaForm({
schema,
values,
@ -588,7 +618,9 @@ export function SchemaForm({
// Which alternative the form is currently showing. Held here because it is a
// property of this rendering, not of the arguments: `values` carries what the
// user typed, and nothing in it names a branch.
const [branchIndex, setBranchIndex] = useState(0);
const [branchIndex, setBranchIndex] = useState(
() => matchBranchIndex(branches, values) ?? 0,
);
// A form reused for another entity can be handed a shorter union, so the
// index is clamped rather than trusted — `resetKey` resets it below, but a
// caller that omits it (the elicitation panels mount fresh) supplies none.
@ -622,8 +654,10 @@ export function SchemaForm({
useValueChange(resetKey, () => {
setEnlargedFields(new Set());
// Which branch is selected belongs to the entity it was chosen for, for the
// same reason enlargement does.
setBranchIndex(0);
// same reason enlargement does — reset to whichever branch the new values
// identify, so the visible selection cannot disagree with what would be
// submitted, and to the first when they identify none.
setBranchIndex(matchBranchIndex(branches, values) ?? 0);
});
// Stable so a field's reporting effect subscribes once, not per render. The
@ -687,13 +721,21 @@ export function SchemaForm({
if (!nextBranch) return;
setBranchIndex(nextIndex);
const nextProperties = nextBranch.schema.properties ?? {};
// Only what the *base* contributed is carried across — a value the outgoing
// branch declared belongs to that branch's shape, and a name the two
// branches type differently would arrive as the wrong type entirely (a `3`
// typed into branch A's number field landing in branch B's checkbox). A
// field the incoming branch pins to a `const` is likewise not carried: the
// branches of a discriminated union share the discriminator's *name* and
// disagree about its value.
const outgoing = new Set(activeBranch?.declaredFields ?? []);
const carried: Record<string, unknown> = {};
for (const [name, fieldSchema] of Object.entries(nextProperties)) {
// A field the incoming branch pins to a `const` is not carried: the two
// branches of a discriminated union share the discriminator's *name* and
// disagree about its value, so keeping what the outgoing branch put
// there would leave the arguments claiming the shape they no longer have.
if (values[name] !== undefined && fieldSchema.const === undefined) {
if (
values[name] !== undefined &&
fieldSchema.const === undefined &&
!outgoing.has(name)
) {
carried[name] = values[name];
}
}

103
clients/web/src/test/core/rootUnion.test.ts
View file

@ -1,5 +1,8 @@
import { describe, it, expect } from "vitest";
import { resolveRootUnion } from "@inspector/core/json/rootUnion.js";
import {
declaresAnyFields,
resolveRootUnion,
} from "@inspector/core/json/rootUnion.js";
const EMAIL = {
type: "object",
@ -76,6 +79,43 @@ describe("resolveRootUnion", () => {
expect(Object.keys(base.properties ?? {})).toEqual(["note"]);
});
it("leaves an unsatisfiable allOf alone rather than dropping it", () => {
// `allOf: [false, …]` admits nothing. Treating the boolean member as a
// no-op and stripping the keyword would render a fillable form for a schema
// that can never be satisfied.
const schema = {
type: "object" as const,
allOf: [
false as unknown,
{ type: "object", properties: { x: { type: "string" } } },
],
};
const { base, branches } = resolveRootUnion(schema);
expect(branches).toEqual([]);
expect(base.properties).toBeUndefined();
expect(base.allOf).toBe(schema.allOf);
});
it("leaves an allOf carrying a $ref alone", () => {
// The referent is not resolved here, so its constraints are unknown rather
// than absent.
const { base } = resolveRootUnion({
type: "object",
allOf: [{ $ref: "#/$defs/Thing" }],
});
expect(base.allOf).toHaveLength(1);
expect(base.properties).toBeUndefined();
});
it("declines a union when the allOf beneath it could not be flattened", () => {
const { branches } = resolveRootUnion({
type: "object",
allOf: [{ $ref: "#/$defs/Thing" }],
anyOf: [EMAIL, SMS],
});
expect(branches).toEqual([]);
});
it("merges allOf branches unconditionally", () => {
const { base, branches } = resolveRootUnion({
type: "object",
@ -128,6 +168,40 @@ describe("resolveRootUnion", () => {
expect(branches[0].declaredFields).toEqual(["id"]);
});
it("declines a union whose branch restates a constraint differently", () => {
// Both apply, so root `minimum: 10` under branch `minimum: 0` is still 10.
// Rendering either side would accept a value the schema rejects.
const { branches } = resolveRootUnion({
type: "object",
properties: { x: { type: "number", minimum: 10 } },
anyOf: [
{ type: "object", properties: { x: { minimum: 0 } } },
{ type: "object", properties: { other: { type: "string" } } },
],
});
expect(branches).toEqual([]);
});
it("tolerates a branch disagreeing only about annotations", () => {
const { branches } = resolveRootUnion({
type: "object",
properties: { x: { type: "number", description: "root" } },
anyOf: [
{
type: "object",
properties: { x: { description: "branch", maximum: 3 } },
},
{ type: "object", properties: { other: { type: "string" } } },
],
});
expect(branches).toHaveLength(2);
expect(branches[0].schema.properties?.x).toEqual({
type: "number",
description: "branch",
maximum: 3,
});
});
it("declines a union whose branch contradicts the root's type for a field", () => {
// `string` under a base `number` describes a value that cannot exist, so
// flattening it would render one type and accept what the schema rejects.
@ -280,4 +354,31 @@ describe("resolveRootUnion", () => {
expect(Object.keys(base.properties ?? {})).toEqual(["a"]);
});
});
describe("declaresAnyFields", () => {
it("sees fields on a union the resolver declines", () => {
// A declined union still HAS fields — reporting none would auto-invoke an
// App tool with `{}` instead of asking for them.
const schema = {
type: "object" as const,
anyOf: [EMAIL, { $ref: "#/$defs/SMS" }],
};
expect(resolveRootUnion(schema).branches).toEqual([]);
expect(declaresAnyFields(schema)).toBe(true);
});
it("sees fields nested a level down", () => {
expect(
declaresAnyFields({
type: "object",
allOf: [{ type: "object", anyOf: [EMAIL, SMS] }],
}),
).toBe(true);
});
it("reports none for a bare object schema", () => {
expect(declaresAnyFields({ type: "object" })).toBe(false);
expect(declaresAnyFields(undefined)).toBe(false);
});
});
});

16
clients/web/src/utils/toolUtils.test.ts
View file

@ -118,6 +118,22 @@ describe("hasInputFields with root composition (#2123)", () => {
).toBe(true);
});
it("sees fields on a union the form declines to flatten", () => {
// Otherwise an App tool with such a schema is treated as input-free and
// invoked immediately with `{}`.
expect(
hasInputFields(
tool({
type: "object",
anyOf: [
{ type: "object", properties: { a: { type: "string" } } },
{ $ref: "#/$defs/Other" },
],
}),
),
).toBe(true);
});
it("still reports no fields for a bare object schema", () => {
expect(hasInputFields(tool({ type: "object" }))).toBe(false);
});

21
clients/web/src/utils/toolUtils.ts
View file

@ -1,5 +1,5 @@
import type { Tool } from "@modelcontextprotocol/client";
import { resolveRootUnion } from "@inspector/core/json/rootUnion.js";
import { declaresAnyFields } from "@inspector/core/json/rootUnion.js";
/**
* Returns the display label for an MCP entity that follows the BaseMetadata
@ -17,19 +17,16 @@ export function resolveDisplayLabel(name: string, title?: string): string {
* one place so the definition of "has fields" stays consistent if it ever
* grows to consider `additionalProperties` etc.
*
* Root composition is resolved first, since a schema declaring its fields on a
* root `allOf`/`oneOf`/`anyOf` has none of its own (#2123) — an App tool with
* such a schema would otherwise launch with empty arguments rather than asking
* for them.
* Root composition counts, since a schema declaring its fields on a root
* `allOf`/`oneOf`/`anyOf` has none of its own (#2123) — an App tool with such a
* schema would otherwise launch with empty arguments rather than asking for
* them. Counted from the composition members directly rather than from a
* resolved union, so a schema whose composition the form declines to flatten
* still reports the fields it has: it renders fewer controls, not none, and
* auto-invoking it would be wrong either way.
*/
export function hasInputFields(tool: Tool): boolean {
const { base, branches } = resolveRootUnion(tool.inputSchema);
return (
Object.keys(base.properties ?? {}).length > 0 ||
branches.some(
(branch) => Object.keys(branch.schema.properties ?? {}).length > 0,
)
);
return declaresAnyFields(tool.inputSchema);
}
/**