-
Notifications
You must be signed in to change notification settings - Fork 508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added auto-refreshing tool list notification handler to client #239
base: main
Are you sure you want to change the base?
Added auto-refreshing tool list notification handler to client #239
Conversation
Added handler for server's notifications/tools/list_changed notification Implemented auto-refresh for tools list when notification is received Added onToolListChanged callback property to provide updated tools to clients Added setToolListChangedCallback convenience method for callback registration Added capability validation to ensure tools support before refreshing Fixed type definitions for proper TypeScript compliance Added error handling for failed refreshes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically refreshing every time this notification is received could accidentally cause a lot of chattiness between client and server. The logic needs to be more nuanced—e.g., including debouncing, and some way to configure the behavior.
Please also do not include unrelated changes, like the reformatted lines, as this makes your PR harder to review.
- added toolrefresh option to be able to enable or disable it - added debounceMs
I've implemented the requested improvements for the tool list refresh feature:
@jspahrsummers These changes should address your concerns. The debouncing mechanism ensures that even if multiple change notifications are received within the debounce window, only one refresh will occur with a configurable debounce. I've considered rate-limiting as well but it seems excessive given that other areas of the code don't have these options yet. |
Added handler for server's notifications/tools/list_changed notification
Implemented auto-refresh for tools list when notification is received
Added onToolListChanged callback property to provide updated tools to clients
Added setToolListChangedCallback convenience method for callback registration
Added capability validation to ensure tools support before refreshing
Fixed type definitions for proper TypeScript compliance
Added error handling for failed refreshes
Motivation and Context
tools/list_changed
notification.Related Issues and discussions:
#205
https://github.com/orgs/modelcontextprotocol/discussions/76
How Has This Been Tested?
Breaking Changes
Types of changes
[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation update
Checklist
[x] I have read the MCP Documentation
[x] My code follows the repository's style guidelines
[x] New and existing tests pass locally
[x] I have added appropriate error handling
[x] I have added or updated documentation as needed
Additional context