[MCP] Add support for streamable HTTP MCP servers #4317
Conversation
Limit mock SSE response to a single use in tests; add debug eprintlns to rmcp_client tests. Fix order of edition in Cargo.toml and enable tokio io-std feature. Move stdio helper into rmcp_test_server, and comment out ServerInfo name/version fields.
gpeal
force-pushed
the
gpeal/rmcp-client-http
branch
from
734d384 to
50fcc9a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
[P1] Stop logging full MCP configuration at error level
The new call to tracing::error!("new mcp_servers: {mcp_servers:?} …) emits the entire MCP server configuration – including bearer_token values – every time the connection manager starts. Because error logs are typically collected in production, this effectively dumps secrets to log files and may trigger noisy alerting even when everything succeeds. Consider removing the log or downgrading it to a non‑sensitive, lower‑level message that redacts credentials.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
|
@codex review it again |
gpeal
changed the title
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
| env: Option<Option<HashMap<String, String>>>, | ||
|
|
||
| url: Option<String>, | ||
| bearer_token: Option<String>, |
There was a problem hiding this comment.
I don't love that we encourage storing this in plaintext, but I admit we don't have a great alternative for people just yet.
There was a problem hiding this comment.
We should consider https://docs.rs/secrecy/latest/secrecy/
There was a problem hiding this comment.
And also https://crates.io/crates/keyring
I don't love that this is plain text either and we'll be able to provide better options over time. I just want to get the simplest thing in now and we can build upon it.
codex-rs/cli/src/mcp_cmd.rs
Outdated
| }); | ||
| let env = match &cfg.transport { | ||
| McpServerTransportConfig::Stdio { env, .. } => env.as_ref().map(|env| { | ||
| env.iter() |
There was a problem hiding this comment.
Why is this not a straight clone? Does it have to be a BTreeMap? Isn't there a .from() or into() you can use instead here?
There was a problem hiding this comment.
This is just indented from the original impl (view the PR without whitespace). I didn't want to make other modifications here
| }), | ||
| McpServerTransportConfig::StreamableHttp { .. } => None, | ||
| }; | ||
| let transport = match &cfg.transport { |
There was a problem hiding this comment.
Why match &cfg.transport twice? You only use the env in the Stdio case.
There was a problem hiding this comment.
Oh this whole env block can be removed because it's embedded in transport
| let mut entry = TomlTable::new(); | ||
| entry.set_implicit(false); | ||
| entry["command"] = toml_edit::value(config.command.clone()); | ||
| match &config.transport { |
There was a problem hiding this comment.
We really need to move all this TOML editing stuff to a separate file.
| let listener = TcpListener::bind("127.0.0.1:0")?; | ||
| let port = listener.local_addr()?.port(); | ||
| drop(listener); | ||
| let bind_addr = format!("127.0.0.1:{port}"); | ||
| let server_url = format!("http://{bind_addr}/mcp"); |
There was a problem hiding this comment.
What's going on here? What happens when we drop(listener)?
There was a problem hiding this comment.
Binding 0 us bind to a port that we know will work so but then we release it so the mcp http server can bind to it
| .env("MCP_STREAMABLE_HTTP_BIND_ADDR", &bind_addr) | ||
| .env("MCP_TEST_VALUE", expected_env_value) |
There was a problem hiding this comment.
Pull out these constants somewhere?
There was a problem hiding this comment.
MCP_STREAMABLE_HTTP_BIND_ADDR is shared between this test and the test server in another crate and they intentionally don't depend on each other. The other is internal to this test so I think it's fine.
| "schemars", | ||
| "server", | ||
| "transport-child-process", | ||
| "transport-streamable-http-client-reqwest", |
There was a problem hiding this comment.
Note the other Cargo.toml files have been updated to use { workspace = true }
| env_var: Option<String>, | ||
| } | ||
|
|
||
| impl ServerHandler for TestToolServer { |
There was a problem hiding this comment.
Is this basically copy/pasted from the previous file? What's the difference?
gpeal
force-pushed
the
gpeal/rmcp-client-http
branch
from
da25ce7 to
268ba77
Compare
This PR adds support for streamable HTTP MCP servers when the
experimental_use_rmcp_clientis enabled.To set one up, simply add a new mcp server config with the url:
It also supports an optional
bearer_tokenwhich will be provided in an authorization header. The full oauth flow is not supported yet.The config parsing will throw if it detects that the user mixed and matched config fields (like command + bearer token or url + env).
The best way to review it is to review
core/srcand thenrmcp-client/src/rmcp_client.rsfirst. The rest is tests and propagating theTransportstruct around the codebase.Example with the Figma MCP:
