Skip to content

Conversation

olaservo
Copy link
Member

@olaservo olaservo commented Oct 8, 2025

The cleanParams function added in 0.17.0 was overly zealous and removes nulls being set by the tool schema as defaults.

Since there are a few opinionated ways to handle validation for tools I also added the established guidelines to the agent instructions, to make it clearer where and when validation should happen and what Inspector vs the server should handle in these scenarios.

Motivation and Context

Fixes #846

How Has This Been Tested?

I followed these test cases using a set of tools created for testing regression scenarios: mcp-maintainer-toolkit@latest

Test 1: Union Type Test Tool (Primary Regression Test)

Tool: unionTypeTest

Test 1a: Default null values are preserved and sent in tools/call

  • Run the unionTypeTest tool without changing any values
  • In 0.17.0 this would remove all null arguments from the tools/call in the History pane
  • In this branch, null defaults are preserved in the tools/call params

Test 1b: Default null values are sent if input is cleared

  • Clear the optionalString field and run the tool again
  • Expected: Tool receives null for optionalString

Test 1c: Null values are sent when user manually enters them

  • Manually type null in the optionalString field
  • Click "Run Tool"
  • Expected: Tool receives null for optionalString

Test 1d: Non-null values override defaults

  • Enter "hello" in optionalString
  • Enter "42" in optionalNumber
  • Check the optionalBoolean checkbox
  • Click "Run Tool"
  • Expected: Message shows "Optional parameters provided: optionalString: "hello", optionalNumber: 42, optionalBoolean: true"

Test 2: Other Default Values

Tool: formatData

Test 2a: String default value preserved

  • Select the formatData tool
  • Verify format field shows "json" as default
  • Click "Copy Input"
  • Expected: JSON contains "format": "json"
  • Leave format as "json", add {} to data field
  • Click "Run Tool"
  • Expected: Data formatted as JSON

Test 2b: Enum default value preserved

Tool: enumDropdownTest

  • Select the enumDropdownTest tool
  • Verify priority defaults to "medium"
  • Verify colorScheme defaults to "auto"
  • Click "Copy Input"
  • Expected: JSON contains both default values
  • Run tool without changing defaults
  • Expected: Tool receives default values

Test 3: Optional Fields Without Defaults (Backward Compatibility)

Tool: getCurrentTime

Test 3a: Empty optional field is omitted

  • Select the getCurrentTime tool
  • Leave timezone field empty
  • Click "Copy Input"
  • Expected: JSON should NOT contain timezone field (omitted entirely)
  • Click "Run Tool"
  • Expected: Success with "(local timezone)" in response

Test 3b: Optional field with value is included

  • Enter "America/New_York" in timezone
  • Click "Copy Input"
  • Expected: JSON contains "timezone": "America/New_York"
  • Click "Run Tool"
  • Expected: Success with "(America/New_York)" in response

Test 4: Required fields preserve empty values for validation

  • Select the strictTypeValidation tool
  • Leave all fields empty
  • Click "Run Tool"
  • Expected: Server validation error (not Inspector error)
  • Expected: Error explains which required fields are missing/invalid

Test 5: Automated Tests

  • cd client && npm test -- paramUtils.test.ts
  • Expected: All 12 tests pass
  • Specifically verify these new tests pass:
    • "should preserve null values when field has default: null"
    • "should preserve default values that match current value"
    • "should omit values that do not match their default"

Breaking Changes

No - this part of the behavior is unintentional.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@cliffhall
Copy link
Member

cliffhall commented Oct 10, 2025

Test 1a

Screenshot 2025-10-10 at 12 44 50 PM Screenshot 2025-10-10 at 12 46 26 PM Screenshot 2025-10-10 at 12 45 29 PM

With the field literally containing the string "null", shouldn't this have sent "null" for optionalString? It seems we parsed the string "null" into a literal null? That doesn't seem right.

@cliffhall
Copy link
Member

Test 1b

Screenshot 2025-10-10 at 12 49 19 PM Screenshot 2025-10-10 at 12 49 56 PM Screenshot 2025-10-10 at 12 50 13 PM

In this case optionalString is not sent at all.

Copy link
Member

@cliffhall cliffhall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, most tests operate as mentioned except one of the two I left comments about.

@cliffhall
Copy link
Member

cliffhall commented Oct 10, 2025

@olaservo with regard to my comment on your test 1a, interestingly, see this recent PR which adds support for nulling a field by adding a null checkbox. Maybe that's the way we set the field to null rather than the undefined that an empty text box would return. If the field default is null we'd have that box checked from the start.

@cliffhall
Copy link
Member

cliffhall commented Oct 10, 2025

@olaservo UPDATE: the Test 1a results of a string "null" in the textbox being sent as actual null is weirder than it first seemed, by a longshot. https://www.loom.com/share/b8c34976ae2545bc987e8ec50dfc7673

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression from 0.16.8 to 0.17 regarding default null values in tools
2 participants