Adds meta parameter and method for LLM hidden info #1376
Conversation
|
@dsp-ant, @LucaButBoring Request you to review this very small change this is my first contribution just exploring the repo as of now. |
src/mcp/client/session.py
Outdated
| self, | ||
| name: str, | ||
| arguments: dict[str, Any] | None = None, | ||
| meta: dict[str, Any] | None = None, |
There was a problem hiding this comment.
Move this to the end for backwards-compatibility — we should have probably added a , *, before, but I'll leave that up to a maintainer to decide if we should start doing that or not (it would also be a breaking change if we started doing that now).
There was a problem hiding this comment.
Done, except the second thing would wait on that for the same
…28/python-sdk into ishan121028/issue-1224
|
LGTM after changes (I do not have merge permissions, however) |
|
Seems related to #1231 |
felixweinberger
added
the
needs maintainer action
|
@dsp-ant Are there any changes required in this, would be great if we can merge this. |
| def test_call_tool_method_signature(): | ||
| """Test that call_tool method accepts meta parameter in its signature.""" | ||
|
|
||
| signature = inspect.signature(ClientSession.call_tool) | ||
|
|
||
| assert "meta" in signature.parameters, "call_tool method should have 'meta' parameter" | ||
|
|
||
| meta_param = signature.parameters["meta"] | ||
| assert meta_param.default is None, "meta parameter should default to None" |
There was a problem hiding this comment.
Remove this test, it shouldn't be needed :)
This is a fragile test which relies entirely on implementation details rather than functionality.
This should be tested via other unit tests, such as one that passes a "meta" parameter. If "meta" didn't exist then that test would fail. Similarly, there should be a unit tests which test the default case instead of doing inspection on the signature.
|
|
||
| def test_call_tool_request_params_construction(): | ||
| """Test that CallToolRequestParams can be constructed with metadata.""" | ||
| from mcp.types import CallToolRequestParams, RequestParams |
There was a problem hiding this comment.
move imports to top of file
|
|
||
| def test_metadata_serialization(): | ||
| """Test that metadata is properly serialized with _meta alias.""" | ||
| from mcp.types import CallToolRequest, CallToolRequestParams, RequestParams |
There was a problem hiding this comment.
move imports to top of file
| return str(self.request_context.request_id) | ||
|
|
||
| @property | ||
| def request_meta(self) -> dict[str, Any]: |
| types.EmptyResult, | ||
| ) | ||
|
|
||
| async def call_tool( |
There was a problem hiding this comment.
add a unit test which tests that the metadata is correctly sent to the server. Currently the unit tests only test the metadata pydantic models and not actual functinoality.
This is a fix for the bug 1224 and implements the parameter and method for the metadata that is passed for cases when the info is to be hidden from LLMs.
Motivation and Context
1224
How Has This Been Tested?
Yes I have tested with
Breaking Changes
Types of changes
Checklist
Additional context