fix(core): deserialize responses as requested result type#717
Open
stationeros wants to merge 1 commit intomodelcontextprotocol:mainfrom
Open
fix(core): deserialize responses as requested result type#717stationeros wants to merge 1 commit intomodelcontextprotocol:mainfrom
stationeros wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
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.
This change fixes unsafe response handling in
Protocol.request()by deserializing the JSON-RPCresultinto the requestedRequestResulttype instead of relying on a direct runtime cast. It also adds a regression test covering the case where the response payload is JSON-compatible with the expected result type but is not the same runtime subtype.Motivation and Context
Protocol.request()currently completes requests with an unchecked cast fromresponse.resulttoT. That is brittle for responses that are deserialized through a broader/result-polymorphic path, including the task-result scenario described in #601.This change makes the request path decode the response payload as the requested type before completing the deferred result. That avoids
ClassCastException-style failures when the wire payload matches the expected schema but the in-memory runtime subtype does not.Fixes #601.
How Has This Been Tested?
Tested at the protocol/unit-test level.
Added/updated
ProtocolTestcoverage for:_metawhen adding a progress token_metawhen none exists_metaunchanged whenonProgressis absentI have not tested this in a real application yet.
Breaking Changes
Potentially yes.
This patch changes
Protocol.request()to aninline/reifiedgeneric function in order to decode the response as the requested type. Existing Kotlin call sites should continue to look the same, but this is still a public API signature change and may require downstream consumers to recompile. It may also affect non-Kotlin/Java interop expectations.Types of changes
Checklist
Additional context
The response path now decodes
response.resultinto the requestedTbefore completing the request, rather than completing withresponse.result as T.The regression test intentionally delivers a
JSONRPCResponsecontaining a differentRequestResultruntime subtype with the same JSON shape. Before this change, that fails because of the unsafe cast. After this change, the payload is decoded into the requested type and succeeds.If we to avoid the public API signature change, the alternative would be to preserve the existing
request()signature and thread an explicit serializer/decode strategy through the internal request/response pipeline instead.