Adding roots changed and initialized notification handlers #1043
Conversation
|
@felixweinberger I'm new to this repo: do I need to do anything that I haven't done to proceed to a review? |
There was a problem hiding this comment.
Hi @gspencergoog thank you for this contribution! And apologies for the time it took to get back to you on this, currently getting a handle on managing all the inbound contributions like yours.
I left some comments - I think adding these handlers is correct, but I wonder if we can handle the ServerSession requirement in a simpler way instead of introducing a global context variable and using introspection.
felixweinberger
added
needs more work
This commit refactors the notification handling logic to eliminate the global context variable and introspection. The `ServerSession` is now explicitly passed to notification handlers, simplifying the control flow and improving explicitness. This addresses the code review feedback to avoid introspection and global state in the low-level server code.
gspencergoog
force-pushed
the
add_root_notification
branch
from
574f760 to
4c2bdb7
Compare
No worries, I know how it is. :-) Hopefully this is closer to what you were expecting now. |
Removes the use of a `global` statement in `tests/shared/test_notifications.py` to resolve a `PLW0603` linting error reported by Ruff. The `serv_sesh` global variable has been replaced with a mutable list (`server_session_ref`) to share the server session object between the `run_server` and `handle_call_tool` functions within the test. This maintains the test's functionality while adhering to better coding practices.
felixweinberger
removed
the
needs more work
felixweinberger
added
needs maintainer action
I've added two server-side notification handlers, one for
InitializedNotificationand one forRootsListChangedNotification.Motivation and Context
This allows an MCP server to fully implement roots support, since once the initialization is complete, it needs to be able to request roots from clients which support them, and it needs to be able to react to changes in the client's list of roots if they change.
How Has This Been Tested?
I wrote new tests for the notifications, and merged it with the existing progress notification tests.
Breaking Changes
No breaking changes were introduced.
Types of changes
Checklist