Repository navigation
feat: add Streamable HTTP transport to MCP host - #164
Flames4fun wants to merge 5 commits into
Conversation
ossumpossum
left a comment
There was a problem hiding this comment.
Approve with nits. The transport is well-scoped and converges cleanly into the existing stdio session/trace/error model, and the security posture is careful: redaction, redirects off, reserved-header rejection. Fallback selection and the "no operational fallback" guarantee hold and are well tested. Two things are worth closing before merge: a discrepancy between the description and the exception-group code (F1), and a gap between the "real loopback" claims and what actually ships as a reproducible test. Neither blocks correctness.
Verification basis: I read the full diff (2801/-102, 11 files) and both test modules, reproduced F1 directly against the function on the PR head (14b726a), and confirmed CI is green on run 30307827273, including both compatibility boundaries (minimum and latest-v1). I did not run the full suite locally.
What holds up
-
Modern/legacy selection is correct across the declared range.
_discover_streamable_http_clientspulls both symbols viagetattr(..., None)guarded by_optional_sdk_callable, and_select_streamable_http_clientprefersstreamable_http_client, falling back tostreamablehttp_clientonly on absence. Selection is by symbol presence, not version strings, which is correct formcp>=1.12.4,<2. The load-bearing empirical claim (min SDK exposes only legacy, latest exposes modern) is asserted by the newmcp-sdk-compatibilitymatrix (expected_api: legacy/modern), and both entries are green in CI. That job is the real guarantor. -
"Never retry legacy after a modern failure" holds.
_open_streamable_http_transportselects once, then dispatches to the modern or legacy context; modern failures re-raiseAdapterError ... from None, and the legacy client is never touched. Pinned bytest_selected_modern_contract_does_not_operationally_fallback_to_legacyandtest_modern_operational_failure_never_falls_back_and_redacts_http_details(assertsrig.legacy_calls == []). -
Redaction is the strongest part of the change.
MCPServerConfig.url/.headersarefield(repr=False)(test_streamable_http_repr_omits_url_and_headers);_transport_metadatareturns{}forstreamable_http, so connection and close events carry no endpoint or credential material; transport, session, and tool errors do not interpolate the raw exception and sever__cause__viafrom None, so the wrapped SDK exception (which carries the URL and headers in its repr) is unreachable from the trace (test_http_session_errors_identify_operation_without_exposing_raw_exception,test_http_cleanup_exception_group_preserves_single_safe_adapter_error). One invariant for follow-ups: this depends on nothing doingstr(url)or attribute access onserver_configinto a log line.repris safe; direct field access is not. A one-line comment on the config fields would keep a future contributor from reintroducing a leak. -
Trace parity holds on success and on the tool-error path. Both transports route through
_connect_mcp_session, so the sequencemcp_connection_initialized → mcp_tools_discovered → mcp_tool_result → mcp_connection_closedis produced identically; asserted intest_streamable_http_modern_path_has_canonical_trace_and_owned_cleanup, and preserved on a raising tool call intest_http_tool_error_uses_shared_event_path_but_redacts_transport_details. The stdio and HTTP metadata sets differ only bycommand(stdio) versus nothing (HTTP), which is intended. -
Redirect policy is a real mitigation.
follow_redirects=Falseis enforced on both paths: directly on the host-owned client, and forced in_legacy_httpx_client_factoryeven if the caller passedTrue(test_legacy_httpx_factory_preserves_kwargs_without_mutating_caller_values). This closes the confused-deputy vector where a configuredAuthorizationorX-Api-Keywould otherwise follow a 3xx to an unintended origin.
Findings I'd like addressed
F1 — Exception-group collapse is broader than the description claims (correctness). _single_adapter_error_from_exception_group walks the group's leaves and returns the sole AdapterError when len(adapter_errors) == 1. It only bails (return None) on a non-Exception leaf (Cancelled/KeyboardInterrupt/SystemExit) or on multiple AdapterErrors. A leaf that is an ordinary Exception but not an AdapterError, such as a RuntimeError raised during cleanup, is neither collected nor treated as disqualifying. So a group of [AdapterError, RuntimeError] collapses to the AdapterError, and because the caller does raise adapter_error from None, the RuntimeError is dropped.
I reproduced this against the function on the PR head:
from agent_harness.mcp_host import _single_adapter_error_from_exception_group
from agent_harness.adapters import AdapterError
grp = BaseExceptionGroup("cleanup", [AdapterError("adapter failed"), RuntimeError("unexpected cleanup failure")])
_single_adapter_error_from_exception_group(grp)
# -> AdapterError('adapter failed') # the RuntimeError is droppedControls behave as expected: [AdapterError, AdapterError] returns None, and [AdapterError] returns the AdapterError. AdapterError subclasses RuntimeError, but a plain RuntimeError is still not an AdapterError, so it slips through.
This contradicts the description, which states the host "deliberately does not collapse ... unrelated implementation failures." Two options: tighten the guard so it returns None when any leaf is an Exception that is not an AdapterError (collapse only when every leaf is an AdapterError and exactly one exists); or soften the description to match the behavior. I'd tighten it, since masking an unexpected cleanup failure behind a possibly-secondary AdapterError is the outcome the rest of this function avoids. A regression test for the [AdapterError, RuntimeError] case, alongside the existing test_exception_group_with_cancellation_is_not_converted_to_adapter_error, would fail today and pass once F1 is resolved.
F2 — SSRF/endpoint reachability is out of scope but should be named in the security notes. _parse_streamable_http_url does solid input hygiene (scheme allowlist, no credentials, no fragment, no control chars), but by design it accepts loopback, RFC1918, and link-local hosts: test_parse_streamable_http_accepts_supported_url_forms_without_rewriting accepts http://192.168.1.25/mcp and http://[::1]:8000/mcp. An operator-supplied url pointing at http://169.254.169.254/… is reachable. This is bounded, since the runtime config is trusted and local, and the design already lists origin allowlists as future work. I'm not asking for a policy engine, only one line in the adapter docs' security section stating that the transport performs no SSRF or allowlist filtering, so users don't infer safety from the redirect and redaction hardening that is present.
Test gaps (non-blocking)
The suite is thorough on branch coverage but leans on deterministic in-process fakes. test_mcp_sdk_compat.py uses the real mcp/httpx but monkeypatches socket.create_connection and httpx.AsyncClient.send to hard-fail, so it verifies the real client accepts the production argument shape and returns an async context manager (a genuine signature check) but never drives a real initialize → list_tools → call_tool. The description's "real modern/legacy loopback execution" is asserted in prose, not shipped as a reproducible test. One opt-in integration-marked test against a localhost FastMCP streamable-http server would anchor the fakes to real SDK behavior and close the untested _normalize_streamable_http_transport_result real-object path; keep default CI hermetic. Minor: a one-line comment on the filesystem_server.py get_type_hints/re-decoration (why it is needed for the compat matrix), and on the latest-v1 canary intent (a red there on a future mcp<2 rename isn't necessarily the contributor's fault), would help the next reader.
Resolve F1 (or reword the description), add the one integration test, and note the SSRF boundary in the docs, and I'm happy to approve.
Disclosure: this review was prepared with AI assistance (Claude Code). I read the diff and both test modules, reproduced the F1 behavior against the PR head myself, and am accountable for its contents.
|
Thanks for the detailed review. I independently reproduced F1 and agree that the current helper can collapse a mixed I’ll also add an explicit user-facing note that the transport does not perform SSRF filtering or endpoint allowlisting and that private, loopback, and link-local endpoints are not blocked. Regarding the loopback test, issue #161 explicitly asks for focused transport-level coverage that does not require a live network service. The modern and legacy loopback executions listed in the PR description were manual local validation rather than committed CI coverage. I’ll clarify that distinction in the description. I’d prefer to keep a live FastMCP integration test optional or as follow-up work unless the code owner @mertsatilmaz wants it included in this PR. |
|
Thanks, that all sounds right. Your F1 framing (collapse only when the complete recursive leaf set contains exactly one On the loopback test: fair. Clarifying that the modern/legacy runs were manual validation rather than committed coverage resolves the over-claim I was flagging. The Appreciate the quick turnaround. |
Fixes #161
Describe the changes you have made in this PR -
This PR adds unauthenticated Streamable HTTP transport support to the real MCP host runtime, alongside the existing stdio transport.
I wanted to keep this change narrow and reviewable: the goal is not to redesign the MCP host, but to add a second transport that converges into the same session lifecycle, trace format, error model, and cleanup guarantees already used by stdio.
Design approach
Before implementing the transport, I considered three possible approaches.
Option A — Legacy API only
Use
streamablehttp_client, which is available at the project’s minimum supported MCP SDK version (1.12.4).This would have been the smallest implementation and would have covered the complete declared dependency range. However, it would also introduce a new feature using an API that is already deprecated in newer MCP v1 releases.
Option B — Modern API only
Use the newer
streamable_http_clientAPI and a host-managedhttpx.AsyncClient.This provides the cleanest current API and clearer HTTP client ownership, but it is not available in MCP
1.12.4. Choosing it exclusively would either break the project’s published compatibility range or require raising the minimum MCP version as part of this PR.Option C — Modern-first compatibility adapter
Prefer
streamable_http_clientwhen the installed SDK exposes it, and usestreamablehttp_clientonly when the modern symbol is absent.I chose this approach because it is the only one that satisfies both sides of the project’s current contract:
mcp>=1.12.4,<2range.The fallback is based strictly on symbol availability. A failed modern connection is treated as a real operational failure and is never retried through the legacy API, since doing so could hide defects or duplicate connection side effects.
Runtime configuration
The MCP runtime configuration now:
transport: streamable_http.url.headers.timeout_seconds.transportexplicit, preserving the parser’s existing behavior.For example, stdio continues to require
command, while Streamable HTTP requiresurland rejects stdio-only fields such ascommand,args,env, andcwd.The URL and configured headers are treated as potentially sensitive configuration and are excluded from the configuration representation.
MCP SDK compatibility
The host now:
streamable_http_clientAPI when available.streamablehttp_clientAPI only when the modern symbol is absent.ClientSessionlifecycle.AdapterErroronly when Streamable HTTP is explicitly requested.A focused Python 3.11 compatibility matrix validates both boundaries of the declared MCP v1 range:
1.12.4mcp>=1.12.4,<2Transport ownership and lifecycle
Transport-specific behavior ends after obtaining the read and write streams.
From that point onward, both transports use the same flow:
ClientSession→ initialize → discover tools → execute tool calls → closeFor the modern API:
httpx.AsyncClient.For the legacy API:
Partial failures during HTTP client creation, transport entry, stream normalization, session entry, initialization, tool discovery, and tool execution all unwind through the existing async resource stack.
Host behavior and trace parity
The host now dispatches through a single transport boundary based on
server_config.transport:stdiouses the existing stdio connector.streamable_httpuses the new HTTP connector.AdapterError.Both transports produce the same canonical trace containers:
mcp_serversmcp_tool_callsmcp_eventsSuccessful runs preserve the existing lifecycle sequence:
mcp_connection_initialized→mcp_tools_discovered→mcp_tool_result→mcp_connection_closedThe existing stdio metadata remains unchanged. HTTP metadata uses the configured server ID and negotiated MCP server information without introducing an artificial command field or exposing the endpoint URL.
Security and error handling
Several defensive decisions were applied deliberately:
X-Api-Keyfrom being forwarded to an unintended redirected origin.AdapterErrormessages rather than interpolating raw HTTP exceptions.Real MCP/AnyIO testing also revealed that cleanup could wrap an already-safe
AdapterErrorinside anExceptionGroup.The host now unwraps that group only when its complete recursive leaf set contains exactly one unambiguous
AdapterError.It deliberately does not collapse:
AdapterErrorinstances.KeyboardInterrupt.SystemExit.This preserves useful failure information instead of selecting an arbitrary exception and potentially hiding a real cleanup defect.
Tests and validation
The implementation includes focused coverage for:
ExceptionGroupnormalization.Real SDK and integration validation included:
1.12.4.--mcp-host-target.AdapterErrorboundary.Validation results:
526 passed, 1 skipped204 passed, 1 skipped204 passed, 1 skipped520 passed, 3 skippedpip check: passed in all isolated environmentsgit diff --check: passedLocal validation used Python 3.12 on Windows. GitHub Actions is configured to validate Python 3.11 on Ubuntu.
The latest-v1 matrix entry intentionally floats within
mcp>=1.12.4,<2, so it can detect future stable MCP v1 releases that become incompatible while still satisfying the project’s declared dependency range.Documentation
This PR also:
--mcp-targetand--mcp-host-target.[Unreleased]inCHANGELOG.md.Out of scope
The PR intentionally does not include:
Those remain follow-up work under the broader MCP host roadmap.
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code or documentation?
If you used AI assistance, briefly describe:
1.12.4and latest-v1 compatibility suites, ordinary no-MCP suite, real modern and legacy loopback simulations, real CLI execution, inaccessible-endpoint error replay, stdio regression tests, Ruff, mypy,pip check, documentation example validation, workflow YAML parsing, andgit diff --check.Checklist before requesting a review
[Unreleased]inCHANGELOG.md