diff --git a/README.md b/README.md index 040edb94a..4f07df864 100644 --- a/README.md +++ b/README.md @@ -458,6 +458,17 @@ npx @modelcontextprotocol/inspector --cli https://my-mcp-server.example.com --me | **Automation** | N/A | Ideal for CI/CD pipelines, batch processing, and integration with coding assistants | | **Learning MCP** | Rich visual interface helps new users understand server capabilities | Simplified commands for focused learning of specific endpoints | +## Tool Input Validation Guidelines + +When implementing or modifying tool input parameter handling in the Inspector: + +- **Omit optional fields with empty values** - When processing form inputs, omit empty strings or null values for optional parameters, UNLESS the field has an explicit default value in the schema that matches the current value +- **Preserve explicit default values** - If a field schema contains an explicit default (e.g., `default: null`), and the current value matches that default, include it in the request. This is a meaningful value the tool expects +- **Always include required fields** - Preserve required field values even when empty, allowing the MCP server to validate and return appropriate error messages +- **Defer deep validation to the server** - Implement basic field presence checking in the Inspector client, but rely on the MCP server for parameter validation according to its schema + +These guidelines maintain clean parameter passing and proper separation of concerns between the Inspector client and MCP servers. + ## License This project is licensed under the MIT License—see the [LICENSE](LICENSE) file for details. diff --git a/client/src/components/ToolsTab.tsx b/client/src/components/ToolsTab.tsx index 50edaf514..740cc5edb 100644 --- a/client/src/components/ToolsTab.tsx +++ b/client/src/components/ToolsTab.tsx @@ -179,7 +179,18 @@ const ToolsTab = ({ onCheckedChange={(checked: boolean) => setParams({ ...params, - [key]: checked ? null : prop.default, + [key]: checked + ? null + : prop.default !== null + ? prop.default + : prop.type === "boolean" + ? false + : prop.type === "string" + ? "" + : prop.type === "number" || + prop.type === "integer" + ? undefined + : undefined, }) } /> diff --git a/client/src/components/__tests__/ToolsTab.test.tsx b/client/src/components/__tests__/ToolsTab.test.tsx index d9237be2e..007ca4f7b 100644 --- a/client/src/components/__tests__/ToolsTab.test.tsx +++ b/client/src/components/__tests__/ToolsTab.test.tsx @@ -177,6 +177,103 @@ describe("ToolsTab", () => { }); }); + it("should support tri-state nullable boolean (null -> false -> true -> null)", async () => { + const mockCallTool = jest.fn(); + const toolWithNullableBoolean: Tool = { + name: "testTool", + description: "Tool with nullable boolean", + inputSchema: { + type: "object" as const, + properties: { + optionalBoolean: { + type: ["boolean", "null"] as const, + default: null, + }, + }, + }, + }; + + renderToolsTab({ + tools: [toolWithNullableBoolean], + selectedTool: toolWithNullableBoolean, + callTool: mockCallTool, + }); + + const nullCheckbox = screen.getByRole("checkbox", { name: /null/i }); + const runButton = screen.getByRole("button", { name: /run tool/i }); + + // State 1: Initial state should be null (input disabled) + const wrapper = screen.getByRole("toolinputwrapper"); + expect(wrapper.classList).toContain("pointer-events-none"); + expect(wrapper.classList).toContain("opacity-50"); + + // Verify tool is called with null initially + await act(async () => { + fireEvent.click(runButton); + }); + expect(mockCallTool).toHaveBeenCalledWith(toolWithNullableBoolean.name, { + optionalBoolean: null, + }); + + // State 2: Uncheck null checkbox -> should set value to false and enable input + await act(async () => { + fireEvent.click(nullCheckbox); + }); + expect(wrapper.classList).not.toContain("pointer-events-none"); + + // Clear previous calls to make assertions clearer + mockCallTool.mockClear(); + + // Verify tool can be called with false + await act(async () => { + fireEvent.click(runButton); + }); + expect(mockCallTool).toHaveBeenLastCalledWith( + toolWithNullableBoolean.name, + { + optionalBoolean: false, + }, + ); + + // State 3: Check boolean checkbox -> should set value to true + // Find the boolean checkbox within the input wrapper (to avoid ID conflict with null checkbox) + const booleanCheckbox = within(wrapper).getByRole("checkbox"); + + mockCallTool.mockClear(); + + await act(async () => { + fireEvent.click(booleanCheckbox); + }); + + // Verify tool can be called with true + await act(async () => { + fireEvent.click(runButton); + }); + expect(mockCallTool).toHaveBeenLastCalledWith( + toolWithNullableBoolean.name, + { + optionalBoolean: true, + }, + ); + + // State 4: Check null checkbox again -> should set value back to null and disable input + await act(async () => { + fireEvent.click(nullCheckbox); + }); + expect(wrapper.classList).toContain("pointer-events-none"); + + // Verify tool can be called with null again + await act(async () => { + fireEvent.click(runButton); + }); + expect(mockCallTool).toHaveBeenLastCalledWith( + toolWithNullableBoolean.name, + { + optionalBoolean: null, + }, + ); + }); + it("should disable button and change text while tool is running", async () => { // Create a promise that we can resolve later let resolvePromise: ((value: unknown) => void) | undefined; diff --git a/client/src/utils/__tests__/paramUtils.test.ts b/client/src/utils/__tests__/paramUtils.test.ts index 3dfd53828..1077d76b7 100644 --- a/client/src/utils/__tests__/paramUtils.test.ts +++ b/client/src/utils/__tests__/paramUtils.test.ts @@ -205,4 +205,112 @@ describe("cleanParams", () => { // optionalField omitted entirely }); }); + + it("should preserve null values when field has default: null", () => { + const schema: JsonSchemaType = { + type: "object", + required: [], + properties: { + optionalFieldWithNullDefault: { type: "string", default: null }, + optionalFieldWithoutDefault: { type: "string" }, + }, + }; + + const params = { + optionalFieldWithNullDefault: null, + optionalFieldWithoutDefault: null, + }; + + const cleaned = cleanParams(params, schema); + + expect(cleaned).toEqual({ + optionalFieldWithNullDefault: null, // preserved because default: null + // optionalFieldWithoutDefault omitted + }); + }); + + it("should preserve default values that match current value", () => { + const schema: JsonSchemaType = { + type: "object", + required: [], + properties: { + fieldWithDefaultString: { type: "string", default: "defaultValue" }, + fieldWithDefaultNumber: { type: "number", default: 42 }, + fieldWithDefaultNull: { type: "string", default: null }, + fieldWithDefaultBoolean: { type: "boolean", default: false }, + }, + }; + + const params = { + fieldWithDefaultString: "defaultValue", + fieldWithDefaultNumber: 42, + fieldWithDefaultNull: null, + fieldWithDefaultBoolean: false, + }; + + const cleaned = cleanParams(params, schema); + + expect(cleaned).toEqual({ + fieldWithDefaultString: "defaultValue", + fieldWithDefaultNumber: 42, + fieldWithDefaultNull: null, + fieldWithDefaultBoolean: false, + }); + }); + + it("should omit values that do not match their default", () => { + const schema: JsonSchemaType = { + type: "object", + required: [], + properties: { + fieldWithDefault: { type: "string", default: "defaultValue" }, + }, + }; + + const params = { + fieldWithDefault: null, // doesn't match default + }; + + const cleaned = cleanParams(params, schema); + + expect(cleaned).toEqual({ + // fieldWithDefault omitted because value (null) doesn't match default ("defaultValue") + }); + }); + + it("should fix regression from issue #846 - tools with multiple null defaults", () => { + // Reproduces the exact scenario from https://github.com/modelcontextprotocol/inspector/issues/846 + // In v0.17.0, the cleanParams function would remove all null values, + // breaking tools that have parameters with explicit default: null + const schema: JsonSchemaType = { + type: "object", + required: ["requiredString"], + properties: { + optionalString: { type: ["string", "null"], default: null }, + optionalNumber: { type: ["number", "null"], default: null }, + optionalBoolean: { type: ["boolean", "null"], default: null }, + requiredString: { type: "string" }, + }, + }; + + // When a user opens the tool in Inspector, fields initialize with their defaults + const params = { + optionalString: null, // initialized to default + optionalNumber: null, // initialized to default + optionalBoolean: null, // initialized to default + requiredString: "test", + }; + + const cleaned = cleanParams(params, schema); + + // In v0.16, null defaults were preserved (working behavior) + // In v0.17.0, they were removed (regression) + // This fix restores the v0.16 behavior + expect(cleaned).toEqual({ + optionalString: null, + optionalNumber: null, + optionalBoolean: null, + requiredString: "test", + }); + }); }); diff --git a/client/src/utils/paramUtils.ts b/client/src/utils/paramUtils.ts index dfc418023..aa538f404 100644 --- a/client/src/utils/paramUtils.ts +++ b/client/src/utils/paramUtils.ts @@ -2,7 +2,7 @@ import type { JsonSchemaType } from "./jsonUtils"; /** * Cleans parameters by removing undefined, null, and empty string values for optional fields - * while preserving all values for required fields. + * while preserving all values for required fields and fields with explicit default values. * * @param params - The parameters object to clean * @param schema - The JSON schema defining which fields are required @@ -14,13 +14,23 @@ export function cleanParams( ): Record { const cleaned: Record = {}; const required = schema.required || []; + const properties = schema.properties || {}; for (const [key, value] of Object.entries(params)) { const isFieldRequired = required.includes(key); + const fieldSchema = properties[key] as JsonSchemaType | undefined; + + // Check if the field has an explicit default value + const hasDefault = fieldSchema && "default" in fieldSchema; + const defaultValue = hasDefault ? fieldSchema.default : undefined; if (isFieldRequired) { // Required fields: always include, even if empty string or falsy cleaned[key] = value; + } else if (hasDefault && value === defaultValue) { + // Field has a default value and current value matches it - preserve it + // This is important for cases like default: null + cleaned[key] = value; } else { // Optional fields: only include if they have meaningful values if (value !== undefined && value !== "" && value !== null) {