-
Notifications
You must be signed in to change notification settings - Fork 176
#293 Add handler for logging level requests in Server #347
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
Conversation
Introduces an internal request handler for LoggingMessageNotification.SetLevelRequest in Server.connect to prevent client errors when copilot logging level requests are received. Returns EmptyRequestResult for these requests.
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.
Pull Request Overview
This PR adds a handler for logging level requests to prevent client errors when GitHub Copilot sends SetLevelRequest messages. The change implements a minimal handler that returns an empty result, avoiding unhandled request exceptions.
Key Changes:
- Added imports for
EmptyRequestResult,ErrorCode, andLoggingMessageNotification - Registered an internal request handler for
LoggingSetLevelmethod inServer.connect()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/Server.kt
Outdated
Show resolved
Hide resolved
Deleted the unused ErrorCode import and clarified a comment regarding Copilot logging level requests in the Server class.
|
Integration test is needed |
devcrocod
left a comment
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.
This PR doesn’t address the logging levels issue globally
In my opinion, in pr should introduce a map on the server side to map log levels by sessionId
also should add setLoggingLevels for the client
By default, logging levels shouldn’t be set, the client should handle that.
And finally, should check the capabilities
| val session = ServerSession(serverInfo, options, instructionsProvider?.invoke()) | ||
|
|
||
| // Add internal logging level handler to prevent client errors from Copilot logging level requests | ||
| session.setRequestHandler<LoggingMessageNotification.SetLevelRequest>( |
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.
Probably worth checking the logging capabilities before setting the level
|
I investigated the issue. It was originally caused by the property initialization order. I also added a handler and updated sendLoggingMessage #358 |
Introduces an internal request handler for LoggingMessageNotification.SetLevelRequest in Server.connect to prevent client errors when copilot logging level requests are received. Returns EmptyRequestResult for these requests.
Motivation and Context
Currently, the SetLevelRequest is not implemented. This leads to errors with GitHub copilot.
How Has This Been Tested?
I tested it with my current MCP integration.
Breaking Changes
No
Types of changes
Checklist