Repository navigation
mcp: forward session state through LoggingTransport - #1434
guglielmo-san merged 6 commits into
Conversation
loggingConn did not implement clientConnection. Client.Connect looks the interface up on the connection and skips the call when the assertion fails, so wrapping a client transport in a LoggingTransport silently dropped the session state update: streamableClientConn never learned the initialize result, so for sessions below 2026-07-28 it sent no Mcp-Protocol-Version header after initialize and never opened the standalone SSE GET stream. modelcontextprotocol#1267 added the server-side forwarder, but both interfaces named their method sessionUpdated with different parameter types, so one wrapper could not implement both. Rename them to clientSessionUpdated and serverSessionUpdated, and have loggingConn forward both, and propagateCancellation, to the delegate when it implements them. TestStreamableClientLoggingTransport fails without the change. Fixes modelcontextprotocol#1328
| body: jsonBody(t, resp(2, &emptyResult{}, nil)), | ||
| wantProtocolVersion: protocolVersion20251125, | ||
| }, | ||
| getKey: { |
There was a problem hiding this comment.
nit: just curious, why only this key has to be a variable? If no reason, I guess this could be written inline for consistency
| _ clientConnection = (*loggingConn)(nil) | ||
| _ serverConnection = (*loggingConn)(nil) | ||
| _ cancellationPropagator = (*loggingConn)(nil) | ||
| ) |
There was a problem hiding this comment.
nit: jsonrpc2.ResponseDropper could also be asserted here
There was a problem hiding this comment.
Good point, added the jsonrpc2.ResponseDropper assertion to the same block in ba5ade3.
Inline the GET request key in the logging transport test, and assert at compile time that loggingConn implements jsonrpc2.ResponseDropper.
The merge in 7b02956 dropped the end of TestStreamableClientLoggingTransport, so mcp/streamable_client_test.go no longer parsed.
|
Thanks @guglielmo-san. The merge of main in 7b02956 dropped the two closing braces at the end of |
|
@Zhuoxi2000 thank you for the contribution! |
loggingConndid not implementclientConnection.Client.Connectlooks the interface up on the connection and skips the call when the assertion fails, so wrapping a client transport in aLoggingTransportsilently dropped the session state update:streamableClientConnnever learned the initialize result, so for sessions below 2026-07-28 it sent noMcp-Protocol-Versionheader after initialize and never opened the standalone SSE GET stream. #1107 covered the header for 2026-07-28 sessions by reading it from the message's_meta, but older sessions and the standalone stream still depended on the hook.#1267 added the server-side
sessionUpdatedforwarder tologgingConn. The client side could not be added the same way: both interfaces named their methodsessionUpdatedwith different parameter types, so one wrapper could not implement both. This change renames them toclientSessionUpdatedandserverSessionUpdated(both unexported), keeps the server forwarder from #1267 under the new name, and adds the client one.propagateCancellation, whichconnectlooks up the same way, is forwarded too. Compile-time assertions keeploggingConnimplementing all three. No exported API changes.Tests:
TestStreamableClientLoggingTransportruns the same client scenario with and without the wrapper. The wrapped subtest fails on main:TestLoggingTransportServerBatchRejectiondoes the same for a stdio server (a JSON-RPC batch must be rejected on a 2025-11-25 session, wrapped or not). It passes on main since mcp: send no response for a cancelled request on 2026-07-28 stdio sessions #1267 and guards the renamed server forwarder.TestLoggingConnForwardsOptionalInterfaceschecks that each optional interface is forwarded, and that a delegate implementing none of them is handled.go test ./...,gofmt -l .,go vet ./...and staticcheck v0.6.1 are clean.Fixes #1328