docs: MCP gateway integration design#82
Conversation
Signed-off-by: maral <maralbahari.98@gmail.com>
maralbahari
requested review from
bbrowning,
franciscojavierarceo,
jiahuei,
leseb,
noobHappylife,
qandrew and
tjtanaa
as code owners
franciscojavierarceo
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
this lgtm as a first cut, let's iterate and see
| ## Implementing MCP and the First Built-in Tool | ||
|
|
||
| ### Step 1: `McpClient` (`mcp/client.rs`) | ||
|
|
There was a problem hiding this comment.
This embeds handler: Option<Arc<dyn GatewayExecutor>> directly in ToolEntry and adds ToolRegistry::dispatch(). PR #83 (currently open) goes the other direction: a separate HashMap<ToolType, Arc<dyn GatewayExecutor>> built at the call site and passed into dispatch_tools. Both are landing simultaneously and they're incompatible — one puts executor ownership in the registry, the other keeps it external. Before either the MCP loop PR or any downstream work can proceed cleanly, we need to pick one model and have the other converge to it. What's the intended resolution here?
| ```rust | ||
| pub struct McpClient { | ||
| inner: Arc<rmcp::service::RunningService<RoleClient>>, | ||
| tool_timeout: Duration, |
There was a problem hiding this comment.
Adding handler to ToolEntry means every existing construction site in ToolRegistry::build() — for Function, WebSearch, FileSearch, CodeInterpreter — needs handler: None. Not a design concern, just a practical heads-up that this touches more of the codebase than the doc implies.
|
|
||
| impl McpClientPool { | ||
| pub async fn from_params(params: &[McpToolParam]) -> Self | ||
| pub async fn from_config(servers: HashMap<String, McpServerEntry>) -> Self |
There was a problem hiding this comment.
build_mcp_registry() constructs an McpHandler for ReadResource using McpClient::placeholder() because that path routes through the pool and never uses the client directly. If placeholder() is ever called on a method that isn't expected to be a no-op, it silently misbehaves rather than failing. Could the client field on McpHandler be Option<Arc<McpClient>> for variants that don't need it, so the absence is explicit in the type rather than hidden behind a sentinel?
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The ordering guarantee needs to be explicit: if the LLM emits parallel tool calls in one turn, tool B blocks on tool A completing because dispatch is sequential. If that's intentional for this phase, document it explicitly so it doesn't get treated as a bug when someone later assumes concurrent dispatch.
| ``` | ||
|
|
||
| `McpClientPool` gains a config-based constructor for gateway-managed servers: | ||
|
|
There was a problem hiding this comment.
When rmcp is added to Cargo.toml, pin the version and flag that modelcontextprotocol/rust-sdk is pre-1.0 — API stability isn't guaranteed across minor bumps, and reviewers should evaluate that risk consciously at add time.
ashwing
left a comment
ashwing
left a comment
There was a problem hiding this comment.
The dispatch model in this doc conflicts with what PR #83 just introduced, and that's the main thread to pull before this moves forward. The other points are smaller — construction site impact, a fragile placeholder pattern, and the new rmcp dependency — but they're all downstream of resolving which dispatch interface wins.
3 participants
Summary
Adds
docs/design/mcp-gateway-integration.mdoutlining the design for integrating MCP into the agentic-api gateway. CoversMcpClient,McpClientPool(HTTP and stdio transports),McpHandler/McpHandlerKind,ToolRegistrydispatch,ResponseAccumulatorextension for tool dispatch, and theexecute_with_mcp()agentic loop. Includes a roadmap for stdio server support and a two-PR split for infrastructure vs execution loop.Test Plan