diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/ToolTestPanel.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/ToolTestPanel.test.tsx index 9ac67563011..f1414a4b104 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/ToolTestPanel.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/ToolTestPanel.test.tsx @@ -160,9 +160,8 @@ describe("ToolTestPanel defaults", () => { }); it("renders inputs for nullable (type-array) schema properties instead of leaving them blank", () => { - // JSON Schema represents an optional/nullable field as an array type, e.g. ["string", "null"] - - // the shape Pydantic's model_json_schema() emits for Optional[str]. A real Azure DevOps MCP tool - // schema (testplan_test_suite_write) declares its optional fields exactly this way. + // A real Azure DevOps MCP tool schema (testplan_test_suite_write) declares its optional + // fields exactly this way. const schema: InputSchema = { type: "object", properties: { @@ -175,10 +174,35 @@ describe("ToolTestPanel defaults", () => { renderPanel(schema); expect(screen.getByLabelText("name")).toHaveValue(""); - expect(screen.getByLabelText("parentSuiteId")).toHaveValue(0); + expect(screen.getByLabelText("parentSuiteId")).toHaveValue(null); expect(screen.getByLabelText("active")).toBeInTheDocument(); }); + it("omits an untouched nullable numeric field from the submitted payload instead of sending a synthetic 0", async () => { + const schema: InputSchema = { + type: "object", + properties: { + parentSuiteId: { type: ["integer", "null"], description: "Optional parent suite id" }, + }, + }; + const onSubmit = vi.fn(); + + render( + , + ); + + await userEvent.click(screen.getByRole("button", { name: "Call Tool" })); + + expect(onSubmit).toHaveBeenCalledWith({}); + }); + it("coerces a nullable integer field to a number on submit, not a string", async () => { const schema: InputSchema = { type: "object", diff --git a/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.test.tsx index 60a808c4496..b5972c1fb12 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.test.tsx @@ -180,11 +180,10 @@ describe("MCPToolArgumentsForm", () => { }); describe("MCPToolArgumentsForm nullable schema types", () => { - it("renders inputs for nullable (type-array) schema properties instead of a plain text fallback", () => { - // JSON Schema represents an optional/nullable field as an array type, e.g. ["integer", "null"] - - // the shape Pydantic's model_json_schema() emits for Optional[int]. Unlike ToolTestPanel, this - // component has a final "else" branch that falls back to a plain text Input, so the bug here - // is a wrong widget (text instead of number), not a missing one. + it("renders a number widget for nullable (type-array) schema properties instead of a plain text fallback", () => { + // Unlike ToolTestPanel, this component has a final "else" branch that falls back to a plain + // text Input, so the pre-fix bug here was a wrong widget (text instead of number), not a + // missing one; the field must also start empty, not a synthetic 0. renderForm({ type: "object", properties: { @@ -193,8 +192,21 @@ describe("MCPToolArgumentsForm nullable schema types", () => { required: [], }); + // Native type="number" input: jest-dom reports an empty one as null, not "". const input = screen.getByLabelText("parentSuiteId"); - expect(input).toHaveValue(0); + expect(input).toHaveValue(null); + }); + + it("omits an untouched nullable numeric field from getSubmitValues instead of sending a synthetic 0", async () => { + const ref = renderForm({ + type: "object", + properties: { + parentSuiteId: { type: ["integer", "null"], description: "Optional parent suite id" }, + }, + required: [], + }); + + await expect(submit(ref)).resolves.toEqual({}); }); it("coerces a nullable integer field to a number in getSubmitValues, not a string", async () => { diff --git a/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.tsx b/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.tsx index 7f06cfbdd4e..0738c05ffc9 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/MCPToolArgumentsForm.tsx @@ -8,6 +8,7 @@ import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@ import { Textarea } from "@/components/ui/textarea"; import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/components/ui/tooltip"; import { MCPTool, InputSchema, InputSchemaProperty } from "./types"; +import { resolveSchemaType, getInitialValueForField } from "./mcpToolSchemaDefaults"; type ToolFormValues = Record; @@ -74,87 +75,6 @@ const labelFor = (key: string, prop: InputSchemaProperty, required: boolean): Re ); -const isPlainObject = (value: unknown): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); - -// JSON Schema allows "type" to be an array (e.g. ["string", "null"]) for a nullable field, the -// shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Every branch below -// that decides a widget or a value conversion from a property's type needs the single effective -// (non-null) type, not the raw field verbatim. -function resolveSchemaType(type: InputSchemaProperty["type"] | undefined): string | undefined { - if (Array.isArray(type)) { - return type.find((t) => t !== "null") ?? type[0]; - } - return type; -} - -function buildArrayItems(items?: InputSchemaProperty | InputSchemaProperty[]): any[] { - if (!items) return []; - if (Array.isArray(items)) { - return items.map((item) => buildDefaultValue(item)).filter((value) => value !== undefined); - } - const itemDefault = buildDefaultValue(items); - return itemDefault !== undefined ? [itemDefault] : []; -} - -function buildDefaultValue(prop?: InputSchemaProperty, overrideDefault?: any): any { - if (!prop) return undefined; - const effectiveDefault = overrideDefault !== undefined ? overrideDefault : prop.default; - const effectiveType = resolveSchemaType(prop.type); - - if (effectiveType === "object") { - const base = isPlainObject(effectiveDefault) ? { ...effectiveDefault } : {}; - if (prop.properties) { - Object.entries(prop.properties).forEach(([childKey, childProp]) => { - base[childKey] = buildDefaultValue(childProp, base[childKey]); - }); - } - return base; - } - - if (effectiveType === "array") { - if (Array.isArray(effectiveDefault)) { - const itemSchema = prop.items; - if (!itemSchema) return effectiveDefault; - if (effectiveDefault.length === 0) { - const sample = buildArrayItems(itemSchema); - return sample.length ? sample : effectiveDefault; - } - if (Array.isArray(itemSchema)) { - return effectiveDefault.map((value, index) => { - const schema = itemSchema[index] ?? itemSchema[itemSchema.length - 1]; - return buildDefaultValue(schema, value); - }); - } - return effectiveDefault.map((value) => buildDefaultValue(itemSchema, value)); - } - if (effectiveDefault !== undefined) return effectiveDefault; - return buildArrayItems(prop.items); - } - - if (effectiveDefault !== undefined) return effectiveDefault; - switch (effectiveType) { - case "integer": - case "number": - return 0; - case "boolean": - return false; - case "string": - default: - return ""; - } -} - -const getInitialValueForField = (prop: InputSchemaProperty): any => { - const defaultValue = buildDefaultValue(prop); - const effectiveType = resolveSchemaType(prop.type); - if (effectiveType === "object" || effectiveType === "array") { - const fallback = effectiveType === "array" ? [] : {}; - return JSON.stringify(defaultValue ?? fallback, null, 2); - } - return defaultValue; -}; - function convertFormValues( values: Record, actualSchema: InputSchema, @@ -391,7 +311,7 @@ const MCPToolArgumentsForm = forwardRef ); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.test.ts b/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.test.ts new file mode 100644 index 00000000000..9c864dffe3b --- /dev/null +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; + +import { buildDefaultValue, getInitialValueForField, resolveSchemaType } from "./mcpToolSchemaDefaults"; + +describe("resolveSchemaType", () => { + it("returns a scalar type unchanged", () => { + expect(resolveSchemaType("string")).toBe("string"); + }); + + it("returns the first non-null entry of a nullable type array", () => { + expect(resolveSchemaType(["integer", "null"])).toBe("integer"); + expect(resolveSchemaType(["null", "boolean"])).toBe("boolean"); + }); + + it("falls back to the first entry when every entry is null", () => { + expect(resolveSchemaType(["null"])).toBe("null"); + }); +}); + +describe("buildDefaultValue", () => { + it("defaults a required (non-nullable) integer with no explicit default to 0", () => { + expect(buildDefaultValue({ type: "integer" })).toBe(0); + }); + + it("defaults a required (non-nullable) boolean with no explicit default to false", () => { + expect(buildDefaultValue({ type: "boolean" })).toBe(false); + }); + + it("defaults a required (non-nullable) string with no explicit default to an empty string", () => { + expect(buildDefaultValue({ type: "string" })).toBe(""); + }); + + it("leaves a nullable integer with no explicit default undefined, not a synthetic 0", () => { + expect(buildDefaultValue({ type: ["integer", "null"] })).toBeUndefined(); + }); + + it("leaves a nullable boolean with no explicit default undefined, not a synthetic false", () => { + expect(buildDefaultValue({ type: ["boolean", "null"] })).toBeUndefined(); + }); + + it("defaults a nullable string with no explicit default to an empty string, unaffected by nullability", () => { + expect(buildDefaultValue({ type: ["string", "null"] })).toBe(""); + }); + + it("still honors an explicit default on a nullable numeric field", () => { + expect(buildDefaultValue({ type: ["integer", "null"], default: 7 })).toBe(7); + }); +}); + +describe("getInitialValueForField", () => { + it("passes through undefined for a nullable numeric field with no default", () => { + expect(getInitialValueForField({ type: ["integer", "null"] })).toBeUndefined(); + }); + + it("JSON-stringifies an object field's computed default", () => { + expect(getInitialValueForField({ type: "object", properties: { a: { type: "string" } } })).toBe( + JSON.stringify({ a: "" }, null, 2), + ); + }); +}); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.ts b/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.ts new file mode 100644 index 00000000000..9080b0ef03e --- /dev/null +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcpToolSchemaDefaults.ts @@ -0,0 +1,88 @@ +import { InputSchemaProperty } from "./types"; + +// JSON Schema allows "type" to be an array (e.g. ["string", "null"]) for a nullable field, the +// shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Every branch that +// decides a widget or a value conversion from a property's type needs the single effective +// (non-null) type, not the raw field verbatim. +export function resolveSchemaType(type: InputSchemaProperty["type"] | undefined): string | undefined { + if (Array.isArray(type)) { + return type.find((t) => t !== "null") ?? type[0]; + } + return type; +} + +const isPlainObject = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + +function buildArrayItems(items?: InputSchemaProperty | InputSchemaProperty[]): any[] { + if (!items) return []; + if (Array.isArray(items)) { + return items.map((item) => buildDefaultValue(item)).filter((value) => value !== undefined); + } + const itemDefault = buildDefaultValue(items); + return itemDefault !== undefined ? [itemDefault] : []; +} + +export function buildDefaultValue(prop?: InputSchemaProperty, overrideDefault?: any): any { + if (!prop) return undefined; + const effectiveDefault = overrideDefault !== undefined ? overrideDefault : prop.default; + const effectiveType = resolveSchemaType(prop.type); + + if (effectiveType === "object") { + const base = isPlainObject(effectiveDefault) ? { ...effectiveDefault } : {}; + if (prop.properties) { + Object.entries(prop.properties).forEach(([childKey, childProp]) => { + base[childKey] = buildDefaultValue(childProp, base[childKey]); + }); + } + return base; + } + + if (effectiveType === "array") { + if (Array.isArray(effectiveDefault)) { + const itemSchema = prop.items; + if (!itemSchema) return effectiveDefault; + if (effectiveDefault.length === 0) { + const sample = buildArrayItems(itemSchema); + return sample.length ? sample : effectiveDefault; + } + if (Array.isArray(itemSchema)) { + return effectiveDefault.map((value, index) => { + const schema = itemSchema[index] ?? itemSchema[itemSchema.length - 1]; + return buildDefaultValue(schema, value); + }); + } + return effectiveDefault.map((value) => buildDefaultValue(itemSchema, value)); + } + if (effectiveDefault !== undefined) return effectiveDefault; + return buildArrayItems(prop.items); + } + + if (effectiveDefault !== undefined) return effectiveDefault; + + // A nullable numeric/boolean field (an array "type") with no explicit default starts empty + // rather than a synthetic 0/false: unlike a string's natural "" default, that value looks + // user-provided, passes the non-empty submission filter, and gets sent to the tool even when + // the field was never touched. + const isNullable = Array.isArray(prop.type); + switch (effectiveType) { + case "integer": + case "number": + return isNullable ? undefined : 0; + case "boolean": + return isNullable ? undefined : false; + case "string": + default: + return ""; + } +} + +export const getInitialValueForField = (prop: InputSchemaProperty): any => { + const defaultValue = buildDefaultValue(prop); + const effectiveType = resolveSchemaType(prop.type); + if (effectiveType === "object" || effectiveType === "array") { + const fallback = effectiveType === "array" ? [] : {}; + return JSON.stringify(defaultValue ?? fallback, null, 2); + } + return defaultValue; +}; diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 24fa70c7f5f..dc1938a08e1 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -296,8 +296,9 @@ export const handleAuth = (authType?: string | null): string => { export interface InputSchemaProperty { // JSON Schema allows an array here (e.g. ["string", "null"]) for a nullable/optional field — // the shape Pydantic's model_json_schema() emits for Optional[str] / str | None. Callers must - // resolve to a single type with resolveSchemaType before branching on it. Optional because a - // property may instead be described purely via anyOf/oneOf, with no top-level type at all. + // resolve to a single type with resolveSchemaType (mcpToolSchemaDefaults.ts / toolCallArguments.ts) + // before branching on it. Optional because a property may instead be described purely via + // anyOf/oneOf, with no top-level type at all. type?: string | string[]; description?: string; properties?: Record; // For nested object properties