Skip to content

Conversation

grimmerk
Copy link
Contributor

@grimmerk grimmerk commented Oct 1, 2025

Rewrites parameter parsing to find callback first, then parse args sequentially by type. Handles edge cases where optional params (description, paramsSchema, annotations) may be undefined.

Fixes cases like:

  • tool(name, undefined, schema, undefined, callback)
  • tool(name, description, undefined, annotations, callback)
  • tool(name, undefined, schema, callback)

Motivation and Context

When tool() is called with undefined in optional parameter positions, the original shift-based parsing logic fails because:

  • typeof undefined !== "object" causes undefined values to be skipped incorrectly
  • Sequential shift() operations lose track of parameter positions

This happens in real-world scenarios when:

  • Parameters come from optional config objects (config.annotations may be undefined)
  • TypeScript strict mode is disabled in consuming projects
  • Dynamic tool registration uses conditional parameters

The fix improves runtime robustness without changing the public API, maintaining backward compatibility while handling edge cases more gracefully.

How Has This Been Tested?

  • ✅ All 79 unit tests pass (77 existing + 2 new)
  • ✅ Added test: tool(name, undefined, schema, callback) - description undefined
  • ✅ Added test: tool(name, desc, undefined, annotations, callback) - paramsSchema undefined
  • ✅ Added test: tool(name, desc, schema, undefined, callback) - annotations undefined
  • ✅ Each test verifies both tool registration and callback execution work correctly
  • ✅ Build passes for both ESM and CJS outputs

Breaking Changes

None. This is a non-breaking internal fix:

  • Public API signatures unchanged
  • Existing valid usage patterns continue to work
  • Only improves handling of edge cases that previously failed

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

Additional context

Implementation approach:

  1. Find callback first (search backwards for last function in args)
  2. Extract all parameters before callback
  3. Parse sequentially with explicit undefined handling:
    • Check if arg matches expected type → assign & advance
    • Check if arg is undefined → skip & advance
    • Otherwise → don't advance (might be next param type)

This callback-first approach is more robust than the original shift-based logic because it establishes a fixed reference point (the callback) before parsing optional parameters.

Error handling:
Added validation to throw clear error if no callback function is found in arguments.

Rewrites parsing to find callback first, then parse args sequentially.
Handles edge cases where optional params may be undefined.

Fixes: tool(name, undefined, schema, undefined, callback)
@grimmerk grimmerk requested review from a team and dsp-ant October 1, 2025 15:58
@grimmerk
Copy link
Contributor Author

grimmerk commented Oct 6, 2025

@dsp-ant, if you think this PR is worthwhile, I can try to resolve the conflict.

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you for working in this! We are moving towards the registerTool methods in this SDK, and tool will be deprecated soon (and thank you, you reminded we need to add deprecation notice! )

grimmerk and others added 2 commits October 6, 2025 18:29
Resolved conflicts:
- src/server/mcp.ts: Integrated callback-first parsing logic with new registerTool method
- src/server/mcp.test.ts: Applied prettier formatting and preserved three new test cases for undefined parameter handling

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Update client.callTool() calls to use correct API signature
- Add type assertions for callResult.content to fix TypeScript strict mode errors

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@grimmerk grimmerk requested a review from a team as a code owner October 6, 2025 12:12
@grimmerk
Copy link
Contributor Author

grimmerk commented Oct 6, 2025

@ihrpr Thanks for the kind introduction to the API change. I’ve resolved the conflict.
I think this PR might still be worthwhile since the deprecated tool method could remain in use for a while.

@grimmerk grimmerk requested a review from ihrpr October 7, 2025 04:55
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.

2 participants