diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index cb9f1df2..94f4990c 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -819,6 +819,24 @@ describe("schemaToForm", () => { expect(missingRequiredFields(schema, {})).toEqual(["constructor"]); }); + it("accepts null only where the schema admits it", () => { + // Branch fields render optional, so a `default: null` on a non-nullable + // required field reaches this check — and `type: "string"` rejects it. + const strict = { + type: "object", + properties: { a: { type: "string" } }, + required: ["a"], + }; + expect(missingRequiredFields(strict, { a: null })).toEqual(["a"]); + + const nullable = { + type: "object", + properties: { a: { type: ["string", "null"] } }, + required: ["a"], + }; + expect(missingRequiredFields(nullable, { a: null })).toEqual([]); + }); + it("checks the root's own required fields when there is no union", () => { const schema = { type: "object", diff --git a/clients/tui/src/utils/schemaToForm.ts b/clients/tui/src/utils/schemaToForm.ts index 196b7357..03224bdd 100644 --- a/clients/tui/src/utils/schemaToForm.ts +++ b/clients/tui/src/utils/schemaToForm.ts @@ -4,6 +4,7 @@ import type { FormStructure, FormSection, FormField } from "ink-form"; import { + admitsNull, isStringEnum, normalizeNullableUnion, } from "@inspector/core/json/nullableUnion.js"; @@ -461,12 +462,25 @@ export function missingRequiredFields( branches.length === 0 ? base : branches[selectedBranchIndex(base, branches, rawValues)]!.schema; + const properties = effective.properties ?? {}; return (effective.required ?? []).filter((name) => { // `hasOwn` first: an argument legally named `constructor` would otherwise // resolve to the inherited one and read as supplied, and the call would go // out without it. if (!Object.hasOwn(decoded, name)) return true; const value = decoded[name]; + if (value === null) { + // `null` counts as supplied only where the schema admits it — the same + // test the web gate applies. A branch field is rendered optional here, so + // a `default: null` on a non-nullable one reaches this check and would + // otherwise send a value the schema rejects. + const property = properties[name]; + return ( + typeof property !== "object" || + property === null || + !admitsNull(property) + ); + } return value === undefined || value === ""; }); } diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index c34a42cf..99d6dc10 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -156,6 +156,31 @@ describe("JSON Utils", () => { ).toEqual({ kind: "a", value: 3 }); }); + it("identifies the branch from the argument names supplied (#2123)", () => { + const undiscriminated: Tool = { + name: "undiscriminated", + inputSchema: { + type: "object", + anyOf: [ + { + type: "object", + properties: { a: { type: "string" }, value: { type: "number" } }, + }, + { + type: "object", + properties: { b: { type: "string" }, value: { type: "boolean" } }, + }, + ], + }, + }; + // `a` belongs to the first branch alone, so `value` is that branch's + // number — falling straight through to cross-branch type agreement would + // drop the coercion and send "3". + expect( + convertToolParameters(undiscriminated, { a: "x", value: "3" }), + ).toEqual({ a: "x", value: 3 }); + }); + it("leaves an ambiguously typed argument uncoerced (#2123)", () => { const ambiguous: Tool = { name: "ambiguous", diff --git a/core/json/jsonUtils.ts b/core/json/jsonUtils.ts index e8b5b9c4..de33de60 100644 --- a/core/json/jsonUtils.ts +++ b/core/json/jsonUtils.ts @@ -1,5 +1,10 @@ import type { Tool } from "@modelcontextprotocol/client"; -import { resolveRootUnion } from "./rootUnion.js"; +import { + narrowBySuppliedNames, + resolveRootUnion, + type RootUnionBranch, + type RootUnionSchema, +} from "./rootUnion.js"; /** * JSON value type used across the inspector project @@ -146,23 +151,34 @@ export function convertParameterValue( * `value=true` into `Number("true")`, i.e. `NaN`. Passing the raw string * through is what this function did for every argument before it existed. */ -function coercionProperties( - base: { properties?: Record }, - branches: { - schema: { properties?: Record }; - declaredFields: string[]; - }[], +function coercionProperties( + base: T, + branches: RootUnionBranch[], params: Record, ): Record { if (branches.length === 0) { return { ...base.properties }; } - const matching = branches.filter((branch) => - matchesConstants(branch.schema.properties ?? {}, params), + // Which branch the call means, from the constants it supplies and then — when + // those leave more than one standing — from the argument NAMES it supplies, + // through the same narrowing the form uses. Without the second step an + // undiscriminated union whose branches type a shared name differently loses + // the coercion for every argument in it, including the ones that identify + // the branch unambiguously. + const candidates = branches + .map((branch, index) => ({ branch, index })) + .filter(({ branch }) => + matchesConstants(branch.schema.properties ?? {}, params), + ) + .map(({ index }) => index); + const selected = narrowBySuppliedNames( + branches, + candidates, + Object.keys(params), ); - if (matching.length === 1) { - return { ...matching[0].schema.properties }; + if (selected !== null) { + return { ...branches[selected]!.schema.properties }; } // `hasOwn`/`fromEntries` rather than `in`/assignment throughout: `properties` @@ -171,12 +187,14 @@ function coercionProperties( const properties: Record = Object.fromEntries( Object.entries(base.properties ?? {}), ); - for (const name of new Set(branches.flatMap((b) => b.declaredFields))) { + const pool = + candidates.length > 0 ? candidates.map((i) => branches[i]!) : branches; + for (const name of new Set(pool.flatMap((b) => b.declaredFields))) { // Only the branches that *declare* the name have an opinion about it — a // branch that merely inherited the root's declaration is not a second, // disagreeing vote. Read through the merged schema so a branch's // specialization of a root property carries the root's keywords too. - const declarations = branches + const declarations = pool .filter((branch) => branch.declaredFields.includes(name)) .map((branch) => branch.schema.properties?.[name]) // A malformed declaration (`properties: { x: null }`) is not a vote about diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index 92247a33..5c77bd28 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -713,23 +713,44 @@ export function selectBranchIndex( // One branch's discriminator matched and no other's did — the plain case. if (agreeing.length === 1) return agreeing[0]; - if (candidates.length === 1) return candidates[0]; - if (candidates.length === 0 || Object.keys(values).length === 0) return null; // Several branches remain — they share the constant that was supplied, or // none was. The values still belong to a shape, so keep looking among the - // candidates: first the branch whose own required fields they supply. + // candidates by the names that were supplied. + return narrowBySuppliedNames( + branches, + candidates, + Object.keys(values).filter(supplied), + ); +} + +/** + * Choose among candidate branches using only *which* argument names were + * supplied — first the branch whose own required fields they cover, then a name + * only one candidate declares. + * + * Split out because two callers need the same answer from different evidence: + * the form holds typed values, while the CLI holds strings it has not coerced + * yet, and a name is a name in both. A name more than one candidate declares is + * ambiguous and says nothing. + */ +export function narrowBySuppliedNames( + branches: RootUnionBranch[], + candidates: number[], + suppliedNames: string[], +): number | null { + if (candidates.length === 1) return candidates[0]; + if (candidates.length === 0 || suppliedNames.length === 0) return null; + const supplied = new Set(suppliedNames); + const satisfied = candidates.filter((index) => { const branch = branches[index]!; const required = branch.schema.required ?? []; const own = required.filter((name) => branch.declaredFields.includes(name)); - return own.length > 0 && own.every(supplied); + return own.length > 0 && own.every((name) => supplied.has(name)); }); if (satisfied.length === 1) return satisfied[0]; - // Then a name only ONE candidate declares: supplying `phone` where only the - // SMS branch declares it names that shape as clearly as a discriminator - // would. A name more than one declares is ambiguous and says nothing. const exclusiveTo = new Map(); for (const index of candidates) { for (const name of branches[index]!.declaredFields) { @@ -737,8 +758,7 @@ export function selectBranchIndex( } } const named = new Set( - Object.keys(values) - .filter(supplied) + suppliedNames .map((name) => exclusiveTo.get(name)) .filter((index): index is number => index !== undefined && index >= 0), );