-
Notifications
You must be signed in to change notification settings - Fork 4k
Omit an empty _meta and empty params from outbound requests #3628
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -359,7 +359,6 @@ async def send_raw_request( | |
| if on_progress is not None: | ||
| # The request id doubles as the progress token, so `_pending[token]` finds `on_progress` directly. | ||
| out_meta["progressToken"] = request_id | ||
| out_params["_meta"] = out_meta | ||
|
|
||
| # buffer=1: a close signal can arrive before the waiter parks in receive(); | ||
| # a WouldBlock later just means the waiter already has its one outcome. | ||
|
|
@@ -386,9 +385,18 @@ async def send_raw_request( | |
| kind=SpanKind.CLIENT, | ||
| attributes={"mcp.method.name": method, "jsonrpc.request.id": str(request_id)}, | ||
| ): | ||
| # SEP-414: inject W3C trace context; `_meta` stays on the wire even with a no-op tracer. | ||
| # SEP-414: inject W3C trace context. | ||
| inject_trace_context(out_meta) | ||
| msg = JSONRPCRequest(jsonrpc="2.0", id=request_id, method=method, params=out_params) | ||
| if out_meta: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): AGENTS.md says any change to an existing API's observable behaviour in 2.x is an explicit maintainer design decision and should generally be avoided. src/mcp/shared/jsonrpc_dispatcher.py now omits Why this was flaggedNothing fails at runtime inside the SDK. The guard is against silent 2.x behaviour drift: handler code written against v2 so far, like Verification: AGENTS.md (base commit) "Branching Model": "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour ... is a design decision a maintainer makes explicitly, and should generally be avoided." |
||
| out_params["_meta"] = out_meta | ||
| else: | ||
| out_params.pop("_meta", None) | ||
| # Leave `params` unset when empty: with `exclude_unset=True` an explicit | ||
| # None would serialize as `"params": null`, which JSON-RPC 2.0 forbids. | ||
| if out_params: | ||
| msg = JSONRPCRequest(jsonrpc="2.0", id=request_id, method=method, params=out_params) | ||
| else: | ||
| msg = JSONRPCRequest(jsonrpc="2.0", id=request_id, method=method) | ||
| # Surface a pre-existing cancellation while the request provably | ||
| # never started; past this point a cancelled write counts as issued. | ||
| await anyio.lowlevel.checkpoint_if_cancelled() | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1482,9 +1482,46 @@ async def on_notify(ctx: DCtx, method: str, params: Mapping[str, Any] | None) -> | |||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.anyio | ||||||||||||
| async def test_send_raw_request_always_carries_meta_on_the_wire(): | ||||||||||||
| """Outbound requests always carry `params._meta` (otel injection per SEP-414); caller-supplied | ||||||||||||
| keys are preserved and the progress token is merged in.""" | ||||||||||||
| async def test_send_raw_request_omits_empty_meta_and_empty_params_on_the_wire(): | ||||||||||||
| """A request with nothing to put in `_meta` carries none, and one left with no params carries no | ||||||||||||
| `params` member. A scripted peer serializes as the transports do (`exclude_unset=True`): a | ||||||||||||
| handler sees `None` for both an absent and a null `params`.""" | ||||||||||||
|
Comment on lines
+1486
to
+1488
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The docstring claims the test verifies that "a handler sees Prompt for AI agents
Suggested change
|
||||||||||||
| c2s_send, c2s_recv = anyio.create_memory_object_stream[SessionMessage | Exception](32) | ||||||||||||
| s2c_send, s2c_recv = anyio.create_memory_object_stream[SessionMessage | Exception](32) | ||||||||||||
| client: JSONRPCDispatcher[TransportContext] = JSONRPCDispatcher(s2c_recv, c2s_send) | ||||||||||||
| on_request, on_notify = echo_handlers(Recorder()) | ||||||||||||
| wire: list[dict[str, Any]] = [] | ||||||||||||
|
|
||||||||||||
| async def peer() -> None: | ||||||||||||
| for _ in range(3): | ||||||||||||
| out = await c2s_recv.receive() | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 nit (optional): AGENTS.md asks that indefinite waits such as Why this was flaggedNothing hangs today: the peer task lives in the same task group as the wrapped Verification: AGENTS.md (base 19e4f2a) Testing section: "Wrap indefinite waits ( |
||||||||||||
| assert isinstance(out, SessionMessage) | ||||||||||||
| assert isinstance(out.message, JSONRPCRequest) | ||||||||||||
| wire.append(json.loads(out.message.model_dump_json(by_alias=True, exclude_unset=True))) | ||||||||||||
| await s2c_send.send(SessionMessage(message=JSONRPCResponse(jsonrpc="2.0", id=out.message.id, result={}))) | ||||||||||||
|
|
||||||||||||
| try: | ||||||||||||
| async with anyio.create_task_group() as tg: | ||||||||||||
| await tg.start(client.run, on_request, on_notify) | ||||||||||||
| tg.start_soon(peer) | ||||||||||||
| with anyio.fail_after(5): | ||||||||||||
| await client.send_raw_request("ping", None) | ||||||||||||
| await client.send_raw_request("tools/list", {"_meta": {}}) | ||||||||||||
| await client.send_raw_request("tools/call", {"name": "t"}) | ||||||||||||
| tg.cancel_scope.cancel() | ||||||||||||
| finally: | ||||||||||||
| for s in (c2s_send, c2s_recv, s2c_send, s2c_recv): | ||||||||||||
| s.close() | ||||||||||||
| assert wire == [ | ||||||||||||
| {"jsonrpc": "2.0", "id": 1, "method": "ping"}, | ||||||||||||
| {"jsonrpc": "2.0", "id": 2, "method": "tools/list"}, | ||||||||||||
| {"jsonrpc": "2.0", "id": 3, "method": "tools/call", "params": {"name": "t"}}, | ||||||||||||
| ] | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.anyio | ||||||||||||
| async def test_send_raw_request_merges_progress_token_into_caller_meta(): | ||||||||||||
| """Caller-supplied `_meta` keys are preserved and the progress token is merged in.""" | ||||||||||||
| seen: list[Mapping[str, Any] | None] = [] | ||||||||||||
|
|
||||||||||||
| async def server_on_request(ctx: DCtx, method: str, params: Mapping[str, Any] | None) -> dict[str, Any]: | ||||||||||||
|
|
@@ -1497,15 +1534,12 @@ async def noop_progress(progress: float, total: float | None, message: str | Non | |||||||||||
| opts: CallOptions = {"on_progress": noop_progress} | ||||||||||||
| async with running_pair(jsonrpc_pair, server_on_request=server_on_request) as (client, *_): | ||||||||||||
| with anyio.fail_after(5): | ||||||||||||
| await client.send_raw_request("a", None) | ||||||||||||
| await client.send_raw_request("b", {"x": 1, "_meta": {"k": "v"}}, opts) | ||||||||||||
| # `_meta` contents depend on the active otel tracer, so pin only what sits beyond the W3C keys. | ||||||||||||
| w3c = {"traceparent", "tracestate"} | ||||||||||||
| assert seen[0] is not None and seen[0].keys() == {"_meta"} | ||||||||||||
| assert set(seen[0]["_meta"].keys()) <= w3c | ||||||||||||
| assert seen[1] is not None and seen[1]["x"] == 1 | ||||||||||||
| assert set(seen[1]["_meta"].keys()) - w3c == {"k", "progressToken"} | ||||||||||||
| assert seen[1]["_meta"]["k"] == "v" | ||||||||||||
| assert seen[0] is not None and seen[0]["x"] == 1 | ||||||||||||
| assert set(seen[0]["_meta"].keys()) - w3c == {"k", "progressToken"} | ||||||||||||
| assert seen[0]["_meta"]["k"] == "v" | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @pytest.mark.anyio | ||||||||||||
|
|
||||||||||||
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.
P2: "Nothing is added to outbound requests" is too broad: progress tokens and caller-supplied non-empty metadata remain outbound without an OpenTelemetry SDK. Say that no tracing fields are added so readers do not infer that progress or metadata disappear.
Prompt for AI agents