fix: update context logging type hints to allow any JSON-serializable data#2445
Closed
cubaseuser123 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Closed
fix: update context logging type hints to allow any JSON-serializable data#2445cubaseuser123 wants to merge 2 commits intomodelcontextprotocol:mainfrom
cubaseuser123 wants to merge 2 commits intomodelcontextprotocol:mainfrom
Conversation
…llow any JSON-serializable data
Contributor
|
Thanks for the PR, but closing in favour of #2366 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The issue here comes down to a frustrating type mismatch: the MCP spec clearly states that log message data can be any JSON-serializable type, but the convenience wrappers for
ctx.info(),ctx.warning(), etc., were strictly enforcingmessage: str. So whenever a dev tried to log a simple Python dictionary or list, the type checker would throw a fit.I traced it down and the good news is the underlying
send_log_messagemethod already handlesAnyproperly without trying to blindly stringify things. The bug was entirely superficial.So I went in and updated the surface-level type hints to bring the API back into alignment with the spec.
Specifically, this updates
message: strtomessage: Anyacross:ctx.log()ctx.info()ctx.debug()ctx.warning()ctx.error()Validation:
I ran the full
pytestsuite againstmcpserver/test_server.pyandclient/test_logging_callback.py— all 90 tests ran cleanly with no regressions to the existing logging behavior.(Side note: I didn't have to touch any serialization logic, since the transport layer already handles the
Anypayload exactly as it should).Let me know if there's anything else you'd like me to tweak before we merge this in! Fixes #397.