feat: add remote MCP gateway support with read_resource as first built-in MCP tool#89
Conversation
…t-in tool on agentic-api Signed-off-by: maral <maralbahari.98@gmail.com>
…rver_url Signed-off-by: maral <maralbahari.98@gmail.com>
ashwing
left a comment
There was a problem hiding this comment.
Read through client/pool/handler/registry + the tests. Comments inline — the main two are test coverage and the silent registry collision; the rest are confirm/observe. Also flagged the #83 merge overlap so we sequence it deliberately.
| )) | ||
| } | ||
|
|
||
| pub enum McpHandlerKind { |
There was a problem hiding this comment.
ReadResourceSpec/ToolCallSpec implement GatewayExecutor::execute only to return Config("...spec-only handler cannot execute") — executor variants that structurally can't execute, i.e. an invalid state the type lets you construct. If spec generation (normalize) were separable from execution, the enum wouldn't carry two arms that always error at runtime. Non-blocking; worth revisiting when the MCP surface settles.
There was a problem hiding this comment.
since we are using the ToolHandler interface for normalize, validate and execution (implemented via GatewayExecutor that implements ToolHandler) . with use of handlers we are normalizing and building the entries to registry and we use them to execute. in case of MCP since we are required to have pool/client connection during normalization we do not need to build that connection the spec kind of Mcp handlers are meant to assist and avoid that connection so that only during registry build we create those connections. I added documentation on top this class to clarify. I think we can revisit the design in future perhaps to cater special cases like MCP. but as off now we are using tool handlers as means of communication between normalize and build.
The framework shipped across vllm-project#80/vllm-project#82/vllm-project#84/vllm-project#85/vllm-project#91 (with vllm-project#83/vllm-project#89 in review) and diverged from the original proposal in several ways. Update the doc to the design-of-record: - Status: Proposal -> Accepted; add as-built preamble. - ToolHandler split into ToolHandler + GatewayExecutor (execute() only applies to gateway-owned types); Pin<Box<dyn Future>> not #[async_trait]; no discover()/event_prefix()/output_item_type() on the trait. - ToolType gains CodexNamespace (6 variants); ResponsesTool shown as the shipped #[non_exhaustive] 7-variant enum incl. namespace + Unknown catch-all and the web_search aliases. - ToolEntry carries handler: Option<Arc<dyn GatewayExecutor>>; dispatch is two layers (per-call registry.dispatch + multi-turn classify_round/ LoopDecision) with a diagram. - LoopDecision trimmed to 4 unit variants; mixed turns resolved by classify_round precedence, not ContinuePartial. - Add Implementation Status (PR mapping) and Future Work (layering ADR, GatewayAccumulator, per-type config, remaining handlers). - Mark Open Questions Q1-Q4 resolved; clarify collision handling is a hard error only for namespace-vs-top-level, warn-level last-write-wins otherwise. Rationale sections (Principles, Alternatives Considered) preserved. Signed-off-by: Ashwin Giridharan <girida@amazon.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
…-server-core rename Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
|
@franciscojavierarceo @ashwing ready to review again. now codex |
ashwing
left a comment
There was a problem hiding this comment.
Took another pass after the rebase — collision warning, the LoopDecision loop, and the Codex read_mcp_resource bridge all look good now. read_mcp_resource works end to end.
One scope thing before this goes in, and a security note inline.
The tools/call discovery path (discovered_tool_handlers → list_tools → tools/call) is all there but nothing calls it, so discovered-tool calling doesn't actually work end to end yet. If this PR is just read_mcp_resource for now (which matches the design doc's first slice), I'd move the discovery bits to the follow-up instead of landing them unwired. If you want them here, they just need to get called + a test. What's the plan?
(Side note, not a problem — stdio looks deliberately config-only, since server_entry_from_param only ever builds Http entries. So a request can't make the gateway spawn a process. Just calling it out so it's not lumped in with the above.)
| }) | ||
| } | ||
|
|
||
| pub async fn discovered_tool_handlers(&self, factory: &McpHandlerFactory) -> Vec<McpDiscoveredHandler> { |
There was a problem hiding this comment.
Nothing calls this on the branch — it's the discovery pass (tools/list → the {server_label}__{tool_name} handlers), and list_tools / the discovered tools/call only run through here. Looks like it wants to run inside build_with_handlers in the Mcp arm and drop its results into entries next to insert_mcp_entry — build's already async and each handler already carries the exposed name + executor, so it'd slot in without touching dispatch. Right now discovered tools/call just can't be reached. Wire it there + a test, or leave it for the follow-up?
There was a problem hiding this comment.
@ashwing yes this is intentional for now cause there will be another follow up PR for this path support. the focus of this PR is interfacing resource/tool spec from rmcp package with first implementation of read_resources which is type of resource spec from rmcp package
| /// # Errors | ||
| /// | ||
| /// Returns [`McpError::Connect`] if the MCP initialization handshake fails. | ||
| pub async fn connect(server_url: &str, headers: Option<HashMap<String, String>>) -> Result<Self, McpError> { |
There was a problem hiding this comment.
validate_request_server_url only checks the hostname string — it never resolves it — and then connect hands the host to the transport, which resolves and connects on its own. So the IP we actually connect to is never checked against the allowlist. Loopback-only default is fine, but the moment someone puts a hostname in AGENTIC_MCP_ALLOWED_HOSTS you've got a rebinding window: the name passes validation, then resolves to whatever at connect time. Either pin the resolved IP between validate and connect, or just document that allowlisted hosts have to be trusted.
There was a problem hiding this comment.
I’ve documented this trust boundary in docs/design/mcp-gateway-integration.md under “HTTP server allowlist security,” with an additional comment next to AGENTIC_MCP_ALLOWED_HOSTS in pool.rs.
Signed-off-by: maral <maralbahari.98@gmail.com>
…isting streaming Signed-off-by: Ashwin Giridharan <girida@amazon.com>
ashwing
left a comment
There was a problem hiding this comment.
Went through the responses — everything from the last two passes is addressed:
- Unhappy-path coverage is there now (unreachable / connection-refused / missing-resource cassettes + the failed-
read_mcp_resourceassertions). - Registry collision is a
tracing::warn!on duplicate name now, matching the function-handler behavior. - DNS-rebinding on allowlisted hosts is documented as a trust boundary (design doc + the note by
AGENTIC_MCP_ALLOWED_HOSTS) — reasonable given the operator opt-in, agreed it doesn't need IP pinning in this PR.
The discovered tools/call / stdio path being unwired is fine as a follow-up — read-resource-first is a sensible first slice, and you've got the interface in place for it.
read_mcp_resource works end to end and CI's green. LGTM.
|
|
||
| let host = url.host_str().ok_or_else(|| "URL must include a host".to_owned())?; | ||
| if is_allowed_request_host(host) { | ||
| return Ok(value.to_owned()); |
There was a problem hiding this comment.
Could we disable redirects for this transport, or validate each redirect target against the same allowlist? This only checks the initial hostname, while the default reqwest client follows redirects, so an allowed MCP endpoint could redirect the gateway to an internal address that would have been rejected here. The allowlist should cover the address we actually connect to.
franciscojavierarceo
left a comment
There was a problem hiding this comment.
The MCP implementation and tests look good. I left one follow-up comment for redirect handling, which is being addressed separately.
Summary
This PR focuses on remote MCP over HTTP and implements read_mcp_resource as the initial built-in MCP gateway tool for HTTP MCP servers. A follow-up PR will add full stdio MCP support, including trusted command and args configuration.
Requests can now pass an MCP tool declaration such as:
{ "type": "mcp", "name": "read_mcp_resource", "server_label": "repo", "server_url": "http://127.0.0.1:8000/mcp" }The gateway normalizes the MCP tool into a model-facing function tool, executes the resulting
read_mcp_resourcecall through an RMCP client, and returns a publicmcp_tool_calloutput item.What Changed
ResponsesTool::Mcpsupport.read_mcp_resourceMCP gateway handler.crates/agentic-core/tests/mcp_tool_test.rs.Codex
read_mcp_resourceMappingCodex normally exposes MCP resources to the model as a local function tool named
read_mcp_resourcewith arguments like:{ "server": "server_label", "uri": "repo://crates/agentic-server-core/tests/cassettes/web_search/gpt_oss_web_search_nonstreaming.yaml" }The gateway now recognizes Codex's
read_mcp_resourcefunction declaration when it includes MCP metadata. The metadata maps the model-providedserverlabel to the MCP server URL, then the gateway registers the function as a gateway-owned MCP entry instead of treating it as a client-owned function.Expected metadata shape:
{ "metadata": { "mcp_servers": { "server_label": { "server_url": "http://127.0.0.1:8000/mcp" } } } }The gateway handles the mapping when this metadata is present.
Running Codex Through Agentic API
Configure Codex with both the agentic-api Responses provider and an MCP server label. For example, in
$CODEX_HOME/config.toml:Start an MCP server separately. For example, the RMCP streamable HTTP example server can be used for local testing:
Any MCP server can be used as long as the Codex
mcp_servers.<your_server_label>.urlvalue points to it and the model callsread_mcp_resourcewith the sameserver_label.Start the agentic-api gateway on
http://localhost:9000/v1, then run Codex with that config:Inside Codex, ask it to use
read_mcp_resourcewith serveryour_server_labeland a resource URI exposed by the MCP server.MCP Server Used For Recording
The cassettes were recorded with the RMCP streamable HTTP example server from
modelcontextprotocol/rust-sdk.The example server was run separately from this repo:
That server listens at:
For recording, the example server exposed a stable MCP resource URI that maps to a YAML cassette file in this repository:
The model was asked to call
read_mcp_resourcefor that URI and then summarize the YAML cassette. This verified that the gateway could connect to the MCP server, read a real resource, feed the result back to the model, and complete the response.How The Cassettes Were Recorded
Run the RMCP example server first, then from the agentic-api repository root:
The successful MCP cassettes are:
The non-streaming cassette contains a completed
mcp_tool_callforread_mcp_resourceand a final assistant message summarizing the web search cassette.The cassette script also records unhappy paths:
These cover the gateway's failure behavior when the MCP server URL is rejected or unavailable, when the URL is loopback but no MCP server is listening, and when the MCP server returns
resource_not_foundfor the requested URI.Validation
Validated with: