diff --git a/AGENTS.md b/AGENTS.md index e54e1346..828828ad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -171,12 +171,19 @@ v2/main/ │ │ # SchemaForm (the Variant picker + the branch-change │ │ # value pruning), TUI schemaToForm (a section per │ │ # branch, its fields forced OPTIONAL since only one -│ │ # alternative applies), and convertToolParameters -│ │ # (which branch's schema types a CLI --tool-arg). -│ │ # Declines a union whose members are not ALL -│ │ # field-carrying objects rather than offering a -│ │ # picker with options that render nothing, and does -│ │ # not interpret `not` at all — #2123; +│ │ # alternative applies — and rendered under PREFIXED +│ │ # names behind a variant select, because ink-form +│ │ # keys values by field name across the WHOLE form, +│ │ # so two branches' `kind` would otherwise be one +│ │ # field; schemaToForm.decodeFormValues translates +│ │ # back on submit), and convertToolParameters (which +│ │ # branch's schema types a CLI --tool-arg). +│ │ # DECLINES rather than half-reads: a union whose +│ │ # members are not ALL field-carrying objects, and a +│ │ # schema carrying BOTH oneOf and anyOf (independent +│ │ # keywords, satisfied together — picking one drops +│ │ # real constraints). Does not interpret `not` at +│ │ # all — #2123; │ │ # schemaLint.ts: tool-schema PORTABILITY lint — │ │ # constructs that are legal JSON Schema and are │ │ # refused or mishandled by real MCP clients (a bare diff --git a/README.md b/README.md index f9a5e5fc..9890e969 100644 --- a/README.md +++ b/README.md @@ -300,11 +300,13 @@ Open the Tools tab and select `echo`. Above the fields is a **Variant** picker l Switching branches drops the values that belonged to the outgoing one. They are no longer on screen, so the user can neither see nor clear them, and submitting them would describe a shape the call is not making. -The **TUI** has the same gap and is worth checking against the same server (`--tui`, then test `echo`). ink-form is static — there is no picker to hide the alternatives behind — so each branch becomes its own **section**, and the fields in it are rendered optional whatever the branch says: only one alternative applies to a call, so requiring them would build a form that can never be submitted. Untouched fields report no value and are dropped before the call, so the sections you skip contribute nothing. +The **TUI** has the same gap and is worth checking against the same server (`--tui`, then test `echo`). ink-form is static — there is no picker to hide the alternatives behind — so each branch becomes its own **section**, preceded by a **Variant** select naming which one the call means. The fields in a branch section are rendered optional whatever the branch says: only one alternative applies to a call, so requiring them would build a form that can never be submitted. + +The sections are not as independent as they look, which is why the select is not cosmetic: ink-form keeps one value object for the whole form, keyed by field name alone, so two branches both declaring `kind` would be **one** field and the later section's initial value would decide what the earlier one submits. Each branch's fields are therefore rendered under a prefixed name and translated back on submit, where every branch but the chosen one is dropped. The **CLI** has no form at all, but the same flattening decides how `--tool-arg` values are typed: a branch's `count: { "type": "number" }` is what turns `--tool-arg count=3` into `3` rather than `"3"`. All three read one helper, [`core/json/rootUnion.ts`](./core/json/rootUnion.ts), so they cannot drift on which schemas they can render. -Two things it deliberately does **not** do. A union whose members are not all field-carrying object schemas is left alone rather than offered as a picker with options that render nothing — the schema falls back to whatever its root `properties` describe. And `not` is not interpreted at all: there is no faithful form for "anything except this". +Three things it deliberately does **not** do, each falling back to whatever the root `properties` describe rather than claiming something untrue. A union whose members are not all field-carrying object schemas is left alone rather than offered as a picker with options that render nothing. A schema carrying **both** `oneOf` and `anyOf` is declined outright: they are independent keywords a value satisfies together, not two spellings of one union, so reading one and dropping the other builds a form that silently omits real constraints — and satisfying both honestly means offering the cross product of their alternatives, which no real schema has yet asked for. And `not` is not interpreted at all: there is no faithful form for "anything except this". #### Unportable tool schemas diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index f1f763c5..cf5afb8c 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect } from "vitest"; -import { schemaToForm } from "../src/utils/schemaToForm.js"; +import { + decodeFormValues, + schemaToForm, + VARIANT_FIELD, +} from "../src/utils/schemaToForm.js"; describe("schemaToForm", () => { it("returns an empty Parameters section when there is no schema", () => { @@ -441,18 +445,43 @@ describe("schemaToForm", () => { "sms", ]); expect(form.sections[0]!.fields.map((field) => field.name)).toEqual([ + VARIANT_FIELD, "note", ]); + }); + + it("names branch fields uniquely, since ink-form scopes by name alone", () => { + // Both branches declare `kind`. Rendered under their real names they + // would be one field, and the later section's initial value would decide + // what the earlier section submits. + const form = schemaToForm(UNION, "union_tool"); expect(form.sections[1]!.fields.map((field) => field.name)).toEqual([ - "kind", - "address", + "__b0__kind", + "__b0__address", ]); + expect(form.sections[2]!.fields.map((field) => field.name)).toEqual([ + "__b1__kind", + "__b1__count", + ]); + }); + + it("offers a variant select listing the alternatives", () => { + const form = schemaToForm(UNION, "union_tool"); + expect(form.sections[0]!.fields[0]).toMatchObject({ + name: VARIANT_FIELD, + type: "select", + initialValue: "0", + options: [ + { label: "email", value: "0" }, + { label: "sms", value: "1" }, + ], + }); }); it("keeps a branch's typed fields typed", () => { const form = schemaToForm(UNION, "union_tool"); expect(form.sections[2]!.fields[1]).toMatchObject({ - name: "count", + name: "__b1__count", type: "integer", }); }); @@ -467,11 +496,74 @@ describe("schemaToForm", () => { it("seeds a branch's discriminator const so it need not be typed", () => { const form = schemaToForm(UNION, "union_tool"); expect(form.sections[1]!.fields[0]).toMatchObject({ - name: "kind", + name: "__b0__kind", initialValue: "email", }); }); + it("prefers a const over a conflicting default", () => { + const form = schemaToForm( + { + type: "object", + properties: { v: { type: "string", const: "a", default: "b" } }, + }, + "const_tool", + ); + expect(form.sections[0]!.fields[0]).toMatchObject({ initialValue: "a" }); + }); + + describe("decodeFormValues", () => { + it("submits the chosen branch's fields under their real names", () => { + expect( + decodeFormValues(UNION, { + [VARIANT_FIELD]: "0", + note: "hi", + __b0__kind: "email", + __b0__address: "a@b.c", + __b1__kind: "sms", + __b1__count: 3, + }), + ).toEqual({ note: "hi", kind: "email", address: "a@b.c" }); + }); + + it("drops the branches the call is not making", () => { + expect( + decodeFormValues(UNION, { + [VARIANT_FIELD]: "1", + __b0__kind: "email", + __b0__address: "a@b.c", + __b1__kind: "sms", + __b1__count: 3, + }), + ).toEqual({ kind: "sms", count: 3 }); + }); + + it("omits a branch field the user never filled", () => { + expect( + decodeFormValues(UNION, { + [VARIANT_FIELD]: "0", + __b0__kind: "email", + }), + ).toEqual({ kind: "email" }); + }); + + it("falls back to the first branch on an unusable selection", () => { + expect( + decodeFormValues(UNION, { + [VARIANT_FIELD]: "nonsense", + __b0__kind: "email", + }), + ).toEqual({ kind: "email" }); + }); + + it("returns the values untouched for a schema with no root union", () => { + const values = { message: "hi" }; + expect(decodeFormValues({ properties: { message: {} } }, values)).toBe( + values, + ); + }); + }); + it("merges a root allOf into the parameters section", () => { const form = schemaToForm( { diff --git a/clients/tui/src/components/ToolTestModal.tsx b/clients/tui/src/components/ToolTestModal.tsx index 5148b93e..3b2d28ca 100644 --- a/clients/tui/src/components/ToolTestModal.tsx +++ b/clients/tui/src/components/ToolTestModal.tsx @@ -5,7 +5,7 @@ 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 { schemaToForm } from "../utils/schemaToForm.js"; +import { decodeFormValues, schemaToForm } from "../utils/schemaToForm.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; interface ToolTestModalProps { @@ -114,9 +114,15 @@ export function ToolTestModal({ { isActive: true }, ); - const handleFormSubmit = async (values: Record) => { + const handleFormSubmit = async (rawValues: Record) => { if (!inspectorClient || !tool) return; + // A root union renders every alternative as its own section, under prefixed + // field names, because ink-form scopes values by name across the whole form + // (#2123). This turns them back into the arguments the server declared: + // the base fields plus the chosen branch's, and nothing from the others. + const values = decodeFormValues(tool.inputSchema, rawValues); + setState("loading"); const startTime = Date.now(); diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 321b10ec..2558dc31 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -70,6 +70,23 @@ interface JsonSchemaObject { discriminator?: { propertyName?: string }; } +/** + * The select that names which alternative of a root union the call is making. + * + * ink-form keeps **one** value object for the whole form, keyed by field name + * alone — sections are visual grouping, not scope. So two branches of a + * discriminated union both declaring `kind` are the *same* field: the later + * section's initial value wins, and filling the first branch's section would + * submit the second branch's discriminator. Prefixing each branch's fields and + * choosing between them explicitly is what makes the alternatives independent. + */ +export const VARIANT_FIELD = "__variant"; + +/** The form-local name a branch's field is rendered under. */ +function branchFieldName(branchIndex: number, name: string): string { + return `__b${branchIndex}__${name}`; +} + /** * Converts a JSON Schema to ink-form structure */ @@ -88,30 +105,88 @@ export function schemaToForm( // could only be called with empty arguments. const { base, branches } = resolveRootUnion(schema); - const sections: FormSection[] = [ - { title: "Parameters", fields: buildFields(base) }, - ]; + const parameters = buildFields(base); + if (branches.length > 0) { + // ink-form is static, so there is no picker that can swap the fields out. + // Every branch is rendered instead, and this select says which one the + // call means — read back by {@link decodeFormValues}, which drops the rest. + parameters.unshift({ + type: "select", + name: VARIANT_FIELD, + label: "Variant", + required: true, + initialValue: "0", + options: branches.map((branch, index) => ({ + label: branch.label, + value: String(index), + })), + } as FormField); + } - // ink-form is static — there is no branch picker to hide the alternatives - // behind — so every branch gets its own section and the user fills the one - // they mean. A branch's fields are rendered **optional** whatever the branch - // says: only one alternative applies to a given call, so requiring them would - // make a form that can never be submitted. An untouched field reports no - // value and is dropped before the call, so the sections the user skipped - // contribute nothing to the arguments. - for (const branch of branches) { + const sections: FormSection[] = [{ title: "Parameters", fields: parameters }]; + + // One section per alternative, its fields **optional** whatever the branch + // says: only one alternative applies to a call, so requiring them would build + // a form that can never be submitted. + branches.forEach((branch, index) => { const ownProperties = Object.fromEntries( - branch.ownFields.map((name) => [name, branch.schema.properties?.[name]]), + branch.ownFields.map((name) => [ + branchFieldName(index, name), + branch.schema.properties?.[name], + ]), ); sections.push({ title: branch.label, fields: buildFields({ properties: ownProperties }), }); - } + }); return { title, sections }; } +/** + * Turn what the form submitted back into the arguments the server expects: + * the base fields, plus the fields of the branch the {@link VARIANT_FIELD} + * select names, under their real property names. + * + * Every other branch's fields are dropped rather than sent — they describe a + * shape this call is not making, and the user filled at most one section. Call + * this on the way out of the form; for a schema with no root union it returns + * the values unchanged, so it is safe to apply unconditionally. + */ +export function decodeFormValues( + schema: JsonSchemaObject | null | undefined, + values: Record, +): Record { + const { branches } = resolveRootUnion(schema ?? {}); + if (branches.length === 0) { + return values; + } + + const raw = values[VARIANT_FIELD]; + const selected = Number(raw); + const branchIndex = + Number.isInteger(selected) && selected >= 0 && selected < branches.length + ? selected + : 0; + + const decoded: Record = {}; + for (const [name, value] of Object.entries(values)) { + // Skip the select itself and every branch's prefixed field; the chosen + // branch's are re-added below under the names the schema declares. + if (name !== VARIANT_FIELD && !name.startsWith("__b")) { + decoded[name] = value; + } + } + for (const name of branches[branchIndex]!.ownFields) { + const value = values[branchFieldName(branchIndex, name)]; + if (value !== undefined) { + decoded[name] = value; + } + } + return decoded; +} + /** Build the ink-form fields for one already-flattened object schema. */ function buildFields(schema: JsonSchemaObject): FormField[] { const fields: FormField[] = []; @@ -204,10 +279,14 @@ function buildFields(schema: JsonSchemaObject): FormField[] { } // Set initial value from default (ink-form FormField allows initialValue for some types). - // A `const` is seeded the same way: it is a one-value enumeration, so the - // only submittable value is already known and the user would otherwise have - // to hand-type a union's discriminator (#2123). - const initialValue = property.default ?? property.const; + // A `const` is seeded the same way and OUTRANKS `default`: it is a + // one-value enumeration, so the only submittable value is already known and + // the user would otherwise have to hand-type a union's discriminator + // (#2123), while `default` is an annotation a schema may set to something + // its own `const` rejects. Tested against `undefined` rather than `??` + // chained, so an explicit `null` default is honored as a value. + const initialValue = + property.const !== undefined ? property.const : property.default; if (initialValue !== undefined) { (field as FormField & { initialValue?: unknown }).initialValue = initialValue; diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index 4ba2cf2e..a35189d8 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -2049,6 +2049,57 @@ describe("SchemaForm multiline strings (#2042)", () => { expect(kind.value).toBe("email"); }); + it("renders a non-string constant read-only too", () => { + // Reached before the number/boolean dispatch, so neither offers a value + // the `const` forbids. + renderWithMantine( + , + ); + expect( + (screen.getByRole("textbox", { name: /N/ }) as HTMLInputElement).value, + ).toBe("7"); + expect( + (screen.getByRole("textbox", { name: /B/ }) as HTMLInputElement).value, + ).toBe("true"); + }); + + it("keeps a const out of an enum select", () => { + // A schema carrying both would otherwise reach the select and offer the + // enum's other members, each of which the `const` rejects. + renderWithMantine( + , + ); + const mode = screen.getByRole("textbox", { + name: /Mode/, + }) as HTMLInputElement; + expect(mode.readOnly).toBe(true); + expect(mode.value).toBe("fast"); + }); + it("renders no picker for a single-branch union but still shows its fields", () => { const schema: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index 495779bc..6d4578ef 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -712,6 +712,35 @@ export function SchemaForm({ const description = fieldSchema.description; const rawValue = resolveValue(values[fieldName], fieldSchema); + // A field pinned to a single value, checked **before** any type dispatch. + // `const` admits exactly one value, so every widget below it — the enum + // select, the number box, the checkbox — would offer values the schema + // forbids, and a schema carrying both `const` and `enum` would otherwise + // reach the select and submit a sibling of the one legal answer. Rendered + // read-only rather than editable for the same reason. + // + // Display only: the value that is *submitted* comes from `values`, seeded + // from the same `const` by `collectSchemaDefaults`, so a non-string + // constant keeps its type on the wire however it is shown here. + if (fieldSchema.const !== undefined) { + const constValue = fieldSchema.const; + return ( + + ); + } + // string with enum if (fieldSchema.type === "string" && fieldSchema.enum) { return ( @@ -750,26 +779,6 @@ export function SchemaForm({ ); } - // A string pinned to a single value. Rendered read-only rather than as an - // editable box: `const` admits exactly one value, so anything the user - // could type into it produces a call the schema rejects. This is what a - // discriminated union's `kind`/`by` field is (#2123), and the picker has - // already set it — but the rule is the keyword's, not the union's, so it - // holds for a lone `const` property too. - if (typeof fieldSchema.const === "string") { - return ( - - ); - } - // plain string if (fieldSchema.type === "string") { const clearButton = rawValue ? ( diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 6ae1e151..522c0bbd 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -104,6 +104,25 @@ describe("JSON Utils", () => { ).toEqual({ note: "hi", count: 42, enabled: true }); }); + it("prefers a branch's specialization of a root property (#2123)", () => { + const specializing: Tool = { + name: "specializing", + inputSchema: { + type: "object", + // The root declares the name but constrains nothing; the branch is + // what says it is a number. + properties: { count: {} }, + anyOf: [ + { type: "object", properties: { count: { type: "number" } } }, + { type: "object", properties: { other: { type: "string" } } }, + ], + }, + }; + expect(convertToolParameters(specializing, { count: "3" })).toEqual({ + count: 3, + }); + }); + it("coerces a value whose schema lives on a root allOf branch (#2123)", () => { const allOfTool: Tool = { name: "allof-tool", diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index 101b211e..21419d51 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -63,13 +63,17 @@ describe("resolveRootUnion", () => { expect(branches[0].schema.anyOf).toBeUndefined(); }); - it("prefers oneOf when a schema carries both", () => { - const { branches } = resolveRootUnion({ + it("declines a schema carrying both oneOf and anyOf", () => { + // Independent keywords a value satisfies together, so reading one and + // dropping the other would build a form missing real constraints. + const { base, branches } = resolveRootUnion({ type: "object", + properties: { note: { type: "string" } }, oneOf: [EMAIL], anyOf: [EMAIL, SMS], }); - expect(branches).toHaveLength(1); + expect(branches).toEqual([]); + expect(Object.keys(base.properties ?? {})).toEqual(["note"]); }); it("merges allOf branches unconditionally", () => { diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 7c84f1e7..f788b5dd 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -382,7 +382,10 @@ describe("root composition (#2123)", () => { ).toEqual({ version: "1" }); }); - it("prefers an explicit default over a const", () => { + it("prefers a const over a conflicting default", () => { + // `default` is an annotation, not a constraint, so a schema may advertise + // one its own `const` rejects — seeding it would submit an invalid value + // through a read-only field. expect( collectSchemaDefaults({ type: "object", @@ -390,7 +393,7 @@ describe("root composition (#2123)", () => { v: { type: "string", const: "a", default: "b" }, }, }), - ).toEqual({ v: "b" }); + ).toEqual({ v: "a" }); }); it("collects defaults from a root allOf", () => { diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index b6ed3b4c..0e29195e 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -139,13 +139,19 @@ export function collectSchemaDefaults( // without this the form would *display* a hoisted default that never // reached the seeded values — the field would submit empty (#1928). const fieldSchema = normalizeNullableUnion(rawSchema); - if (fieldSchema.default !== undefined) { - result[fieldName] = fieldSchema.default; - } else if (fieldSchema.const !== undefined) { + if (fieldSchema.const !== undefined) { // `const` is a one-value enumeration, so the only submittable value is // already known — seeding it spares the user hand-typing a discriminator - // the schema has fixed (#2123), and matches what a `default` would do. + // the schema has fixed (#2123). + // + // It outranks `default`, which JSON Schema defines as an annotation + // rather than a constraint: a schema may advertise a default its own + // `const` rejects, and seeding that would submit an invalid argument + // through a field rendered read-only, leaving the user no way to correct + // it. result[fieldName] = fieldSchema.const; + } else if (fieldSchema.default !== undefined) { + result[fieldName] = fieldSchema.default; } else if (fieldSchema.type === "object" && fieldSchema.properties) { const nested = collectSchemaDefaults(fieldSchema); if (Object.keys(nested).length > 0) { diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index 2df51dbd..da9c51a2 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -142,10 +142,22 @@ export function convertToolParameters( // branch's schema, and a name two branches type differently keeps the first, // which is no worse than the untyped passthrough it replaces. const { base, branches } = resolveRootUnion(tool.inputSchema ?? {}); - const properties: Record = { ...base.properties }; - for (const branch of branches) { - for (const name of branch.ownFields) { - properties[name] ??= branch.schema.properties?.[name]; + // Start from the FIRST branch's merged schema rather than from the base: a + // branch may *specialize* a property the root also declares (root + // `count: {}`, branch `count: { type: "number" }`), and merging branch-last + // is what gives the typed declaration — starting from the base would keep the + // untyped one and send `count=3` as `"3"`, the very coercion this restores. + // Later branches then contribute only names not seen yet, so first-branch + // precedence matches what the web form seeds and what a name two branches + // type differently resolves to. + const properties: Record = { + ...(branches[0]?.schema.properties ?? base.properties), + }; + for (const branch of branches.slice(1)) { + for (const [name, schema] of Object.entries( + branch.schema.properties ?? {}, + )) { + properties[name] ??= schema; } } diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index b73f6ba8..88f4c008 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -193,8 +193,17 @@ function branchLabel( * `branches` only when **every** member is an object schema carrying fields: * a union mixing renderable and unrenderable members would give a picker * options that show nothing, and the whole point of this module is to stop - * producing a form that cannot express the call. `oneOf` wins when a schema - * carries both, being the stricter of the two. + * producing a form that cannot express the call. + * + * A schema carrying **both** `oneOf` and `anyOf` is declined rather than + * half-read. The two are independent keywords a value must satisfy *together*, + * not two spellings of one union, so picking one and dropping the other builds + * a form that silently omits real constraints — worse than the empty form this + * module exists to replace, because it looks complete. Satisfying both honestly + * means offering the cross product of their alternatives, which no real schema + * has yet asked for and which produces a picker whose option labels are pairs; + * until something does, declining leaves the root `properties` rendering + * unchanged and claims nothing that is not true. */ export function resolveRootUnion( schema: T, @@ -208,6 +217,9 @@ export function resolveRootUnion( ); const base = withoutComposition(merged); + if (schema.oneOf !== undefined && schema.anyOf !== undefined) { + return { base, branches: [] }; + } const members = schema.oneOf ?? schema.anyOf ?? []; const branches = members.map(toBranch); if (