fix: revisit MCP support, remove MCP resources, support only discover and execute native tools [1/2] by maralbahari · Pull Request #138 · vllm-project/agentic-api

maralbahari · 2026-07-21T14:58:10Z

Summary

This PR revisits the gateway's MCP support following a comparison with the OpenAI Responses MCP contract.

The previous implementation was largely based on Codex behavior. Codex maintains its own MCP client lifecycle and supports application-controlled MCP resources through client-side operations such as read_mcp_resource. That behavior does not map directly to the gateway's OpenAI-compatible type: "mcp" contract.

For simplicity and a clearer responsibility boundary, the gateway will not support MCP resources for now. Resource discovery, selection, and reading should remain client-controlled behavior in applications such as Codex and Claude Code. Resource support can be revisited later as a separate host-facing context design.

The updated architecture is documented in docs/design/mcp-gateway-integration.md.

This PR:

Removes the synthetic read_mcp_resource function bridge and resources/read execution.
Removes the Codex-specific MCP resource metadata conversion.
Removes the obsolete MCP resource tests, cassettes, fixture, and recorder script.
Treats a client-declared function named read_mcp_resource as an ordinary client-executed function.
Accepts OpenAI-style type: "mcp" declarations using server_label and connection information without a client-supplied tool name.
Connects to request-declared MCP servers and discovers their tools through tools/list.
Applies allowed_tools filtering to discovered tools.
Normalizes discovered MCP tools into model-visible function tools.
Stores discovered tools and their handlers in the request-scoped ToolRegistry.
Caches discovered handlers by server_label.
Executes model-selected MCP tools through tools/call.
Preserves the original MCP server and tool identity during internal execution.
Rejects duplicate MCP server labels and empty discovered tool sets.
Supports request headers and bearer authorization for remote MCP servers.

This PR intentionally retains the existing mcp_tool_call public output item and streaming lifecycle. A follow-up PR will align public output items and SSE events with OpenAI's mcp_call contract.

Important Note:

MCP name collision handling: MCP declarations no longer expose the non-standard mcp.name field. MCP internal names are derived only after tool discovery, so Codex namespace resolution does not inspect MCP names. During registry construction, discovered MCP entries are checked against all registered tools, rejecting collisions across MCP servers and with existing tool entries regardless of declaration order.

Test Plan

cargo check --workspace
cargo test --workspace
cargo clippy --all-targets -- -D warnings
cargo fmt -- --check

…ute native tools Signed-off-by: maral <maralbahari.98@gmail.com>

Signed-off-by: maral <maralbahari.98@gmail.com>

ashwing

Went through this end-to-end against the OpenAI type:mcp contract and our Messages side. Direction is right — rebasing on server_label + tools/list + allowed_tools + tools/call and dropping the synthetic read_mcp_resource bridge is the correct contract, and the executor came out protocol-neutral (our Messages stream loop already consumes ToolRegistry::dispatch). A few things worth resolving before merge; details inline plus one below.

Output item type vs the design doc (output.rs is outside this diff so noting here): the code emits type: "mcp_tool_call" (fields server/tool/result), but the design doc specifies OpenAI's mcp_call (server_label/name/output) and states "The current gateway work implements the mcp_call lifecycle" (mcp-gateway-integration.md:59) — which doesn't match what ships. Either reconcile the doc to call mcp_tool_call the interim shape pending #139, or land the mcp_call rename here. With no external consumers yet, renaming now avoids shipping a non-conformant item type that clients parse and then breaks under them in #139.

ashwing · 2026-07-21T20:05:52Z

@@ -29,7 +29,6 @@ pub enum McpOperation {
    Connect,


MCP discovery (tools/list) runs inside build_with_handlers on the request hot path with a 30s connect + 60s list timeout, and a failure propagates via ? (engine.rs), failing the entire turn — including any healthy servers declared alongside. That's asymmetric with the tool-call path, whose contract is "a hung/failed tool becomes an error fed back to the model, never a whole-request failure." A client can declare a black-hole server_url (e.g. an allowlisted localhost that accepts the socket but never handshakes) and hold a handler slot ~90s per dead server. Worth deciding explicitly: degrade (drop the unreachable server, surface it as an error item) vs fail-fast with a tighter discovery deadline + documented rationale. Servers are also discovered sequentially in registry.rs, so N dead servers stack additively — try_join_all would bound it.

understood the concern the discovery timeout and concurrency need dedicated tests and policy, so we can address them as the next follow-up after #139 while keeping this PR and #139 focused on the mcp_call contract first to avoid complication.

ashwing · 2026-07-21T20:05:52Z

        .is_some()
    {
-        tracing::warn!(name = %param.name, "duplicate MCP tool name — previous definition overwritten");
+        tracing::warn!(name = %internal_name, "duplicate discovered MCP tool name — previous definition overwritten");


Cross-server internal-name collision is silently resolved by overwrite. internal_mcp_tool_name's dedup map is created fresh per server (handler.rs:113), so it only guards collisions within one server. __ is legal in both server_label and tool names and is also the mcp__{server}__{tool} delimiter, so two distinct (server, tool) pairs can collide — e.g. label foo + tool bar__baz and label foo__bar + tool baz both normalize to mcp__foo__bar__baz. Here that just warns and overwrites, so the model sees one tool routing to whichever server registered last — a silent misroute that sends arguments to the wrong server. You already hard-reject duplicate server_labels a few lines up; extending that to derived names (a shared dedup map across servers, or escalating this warn to a Config error) would close it.

Fixed cross-server MCP tool-name collisions.

ashwing · 2026-07-21T20:05:52Z

+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub allowed_tools: Option<Vec<String>>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub require_approval: Option<String>,


require_approval is parsed but never read in execution, so a client sending require_approval: "always" gets silent auto-execution of remote MCP tools — the inverse of what they asked for. The full approval loop (mcp_approval_request/response) can wait for a later PR, but accept-and-ignore on a safety-gating field shouldn't ship: I'd reject with a clear "approval gating not yet supported" until it's honored. Same accept-and-ignore class applies to connector_id (modeled, never consumed).

fixed rejected unsupported MCP approval and connector options.

ashwing · 2026-07-21T20:05:52Z

+}
+
+#[derive(Deserialize)]
+#[serde(deny_unknown_fields)]


deny_unknown_fields with no extra capture makes this the strictest of the tool params — FunctionToolParam/CustomToolParam use #[serde(flatten)] extra. MCP is the one tool type OpenAI extends most often (authorization, connector_id, read_only all arrived incrementally), so this is the wrong place to be strictest: server_description (a currently-valid OpenAI field) already 4xxes here, and the object forms of allowed_tools ({tool_names:[...]}) and require_approval are rejected rather than tolerated. For a gateway claiming OpenAI as ground truth I'd flip to capture-and-preserve so a new OpenAI field degrades to ignore instead of hard-fail.

Removed McpToolParamWire and deny_unknown_fields; McpToolParam now ignores unknown fields, preventing new OpenAI MCP fields from causing 4xx errors.

ashwing · 2026-07-21T20:05:52Z

-                    },
-                    handler: Arc::new(factory.tool_call(Arc::clone(client))),
-                });
+        let mut internal_names = HashMap::new();


Forward-looking note, not a blocker: the MCP executor here (McpClient/McpHandler/dispatch) is nicely protocol-neutral and our Messages stream loop already calls ToolRegistry::dispatch — but discovery is triggered from a ResponsesTool::Mcp walk with a &mut write-back into the payload, and public-item construction lives only in gateway.rs (Responses). When #139 does the mcp_call rename, could the identity/serialization land in tool/mcp/handler.rs (neutral) rather than only gateway.rs? Keeps it reusable if the Messages path eventually grows gateway-owned MCP, and avoids a second hand-parallel construction.

Absolutely, the follow-up MCP call PR keeps identity resolution and McpCall output-item construction in tool/mcp/handler.rs. executor/gateway.rs only handles the Responses-specific lifecycle and SSE envelope. This keeps the MCP execution and public-item construction reusable if the Messages path later supports gateway-owned MCP tools.

maralbahari · 2026-07-22T05:56:50Z

Output item type vs the design doc (output.rs is outside this diff so noting here): the code emits type: "mcp_tool_call" (fields server/tool/result), but the design doc specifies OpenAI's mcp_call (server_label/name/output) and states "The current gateway work implements the mcp_call lifecycle" (mcp-gateway-integration.md:59) — which doesn't match what ships. Either reconcile the doc to call mcp_tool_call the interim shape pending #139, or land the mcp_call rename here. With no external consumers yet, renaming now avoids shipping a non-conformant item type that clients parse and then breaks under them in #139.

@ashwing I would still prefer to keep the mcp_call emitting into #139 even renaming would be adding more file changes to this PR and makes it larger.

I will address your following comments in a bit and push commit. Thanks ashwin

…leanup Signed-off-by: maral <maralbahari.98@gmail.com>

Signed-off-by: maral <maralbahari.98@gmail.com>

maralbahari added 2 commits July 21, 2026 22:40

fix: revisit MCP support, remove MCP resources, and discover and exec…

6e69900

…ute native tools Signed-off-by: maral <maralbahari.98@gmail.com>

update mcp gateway support

b35f56d

Signed-off-by: maral <maralbahari.98@gmail.com>

maralbahari requested review from bbrowning, franciscojavierarceo, jiahuei, leseb, noobHappylife, qandrew and tjtanaa as code owners July 21, 2026 14:58

maralbahari changed the title ~~fix: revisit MCP support, remove MCP resources, support only discover and execute native tools~~ fix: revisit MCP support, remove MCP resources, support only discover and execute native tools [1/2] Jul 21, 2026

ashwing reviewed Jul 21, 2026

View reviewed changes

maralbahari added 4 commits July 22, 2026 15:17

address PR about name collision and tighter param contract and some c…

6ca6166

…leanup Signed-off-by: maral <maralbahari.98@gmail.com>

Merge remote-tracking branch 'origin/main' into fix-mcp-tool-call-1

2643942

fix: validate MCP tool registry name collisions

2897b86

Signed-off-by: maral <maralbahari.98@gmail.com>

clean code

3eedc96

Signed-off-by: maral <maralbahari.98@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: revisit MCP support, remove MCP resources, support only discover and execute native tools [1/2]#138

fix: revisit MCP support, remove MCP resources, support only discover and execute native tools [1/2]#138
maralbahari wants to merge 6 commits into
vllm-project:mainfrom
EmbeddedLLM:fix-mcp-tool-call-1

maralbahari commented Jul 21, 2026 •

edited

Loading

Uh oh!

ashwing left a comment •

edited

Loading

Uh oh!

ashwing Jul 21, 2026

Uh oh!

maralbahari Jul 22, 2026

Uh oh!

ashwing Jul 21, 2026

Uh oh!

maralbahari Jul 22, 2026

Uh oh!

ashwing Jul 21, 2026

Uh oh!

maralbahari Jul 22, 2026

Uh oh!

ashwing Jul 21, 2026

Uh oh!

maralbahari Jul 22, 2026

Uh oh!

ashwing Jul 21, 2026

Uh oh!

maralbahari Jul 22, 2026

Uh oh!

maralbahari commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

maralbahari commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

ashwing left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maralbahari commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maralbahari commented Jul 21, 2026 •

edited

Loading

ashwing left a comment •

edited

Loading