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]#138maralbahari wants to merge 11 commits into
Conversation
…ute native tools Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
maralbahari
changed the title
There was a problem hiding this comment.
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.
| @@ -29,7 +29,6 @@ pub enum McpOperation { | |||
| Connect, | |||
There was a problem hiding this comment.
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.
| .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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed cross-server MCP tool-name collisions.
| #[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>, |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
fixed rejected unsupported MCP approval and connector options.
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| #[serde(deny_unknown_fields)] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed McpToolParamWire and deny_unknown_fields; McpToolParam now ignores unknown fields, preventing new OpenAI MCP fields from causing 4xx errors.
| }, | ||
| handler: Arc::new(factory.tool_call(Arc::clone(client))), | ||
| }); | ||
| let mut internal_names = HashMap::new(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@ashwing I would still prefer to keep the 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>
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
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
I found a few issues that look worth addressing before this lands, mostly around approval defaults, credential handling, request-scoped filtering, and preserving MCP discovery errors.
| "MCP connector_id is not supported; configure server_url instead".to_owned(), | ||
| )); | ||
| } | ||
| if !matches!(param.require_approval.as_deref(), None | Some("never")) { |
There was a problem hiding this comment.
I think this needs to fail closed. When require_approval is omitted, the OpenAI behavior is to require approval, but this makes omission equivalent to "never" and lets the MCP call run immediately. Since the approval flow isn't implemented yet, could we reject an omitted policy and require callers to opt out explicitly with "never"?
There was a problem hiding this comment.
Omitted policies now fail closed, requiring callers to explicitly set "never" until approval gating is supported.
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub headers: Option<HashMap<String, String>>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub authorization: Option<String>, |
There was a problem hiding this comment.
Can we keep these credentials out of persisted tool metadata? effective_tools is stored for stateful responses and conversations, then inherited during continuation. As written, that means the bearer token—and potentially auth headers—gets written to storage and can be replayed without the caller sending it again. I think runtime credentials need to be separated or redacted before persistence.
There was a problem hiding this comment.
added redacted logic before persistence.
| "MCP declaration requires a non-empty server_label".to_owned(), | ||
| )); | ||
| } | ||
| if let Some(cached) = self.mcp.get(server_label) { |
There was a problem hiding this comment.
This early return skips the request's allowed_tools filtering for cached or pre-registered handlers. For example, a request that only allows read could still receive a previously registered delete handler. Could we apply the allowlist to the cached set as well and reject it if nothing remains?
There was a problem hiding this comment.
request allowlists are now applied to cached and pre-registered MCP handlers, with empty results rejected.
| client: Arc<McpClient>, | ||
| allowed_tools: Option<&[String]>, | ||
| ) -> Vec<McpDiscoveredHandler> { | ||
| let tools = match client.list_tools().await { |
There was a problem hiding this comment.
Could we preserve the tools/list error here instead of turning it into an empty result? Right now a server timeout or discovery failure becomes an "empty allowed tool set" configuration error, so callers get a 400 and the useful upstream cause is lost. This seems like it should propagate as an execution/tool error instead.
There was a problem hiding this comment.
tools/list failures now preserve and return the original upstream cause as a tool execution error.
ashwing
left a comment
ashwing
left a comment
There was a problem hiding this comment.
A few things from a fresh pass, beyond the inline notes:
SSRF via allowlisted hostnames (tool/mcp/pool.rs validate_request_server_url + tool/mcp/client.rs:283 pinned_http_client): the allowlist matches on the host string, but pinned_http_client then resolves the domain and pins whatever DNS returns without checking the resolved addresses aren't loopback-adjacent / RFC1918 / 169.254.169.254. The pinning stops rebinding between validate and connect, but an allowlisted name that resolves to an internal address still connects — the existing test only rejects the literal metadata IP, not a domain resolving to it. Since this is the trust boundary, worth rejecting resolved addresses in private/link-local ranges unless explicitly opted in.
#139 rename scope (types/io/output.rs McpToolCall): the item ships server/tool/result; OpenAI's mcp_call uses server_label/name/output. Flagging so #139 covers the field names too, not just the mcp_tool_call->mcp_call type tag — otherwise clients coding to the OpenAI schema still break after the rename.
Minor: the streaming-accumulator test fixture (executor/accumulator.rs:611-614) still emits tool:"read_mcp_resource" items — harmless test data, but it's the exact shape this PR removes, so worth updating so it doesn't imply the path survives.
| #[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>, |
There was a problem hiding this comment.
require_approval is typed Option<String>, but OpenAI also sends the object form {"never":{"tool_names":[...]}}. That object fails McpToolParam deserialization, and because ResponsesTool has an other -> Unknown arm, the whole MCP declaration silently degrades to Unknown and the server is dropped with no error. A small enum (string | object) would handle both forms and avoid the silent drop.
There was a problem hiding this comment.
as per documentation https://developers.openai.com/api/docs/guides/tools-connectors-mcp
the first example is:
from openai import OpenAI
client = OpenAI()
resp = client.responses.create(
model="gpt-5.6",
tools=[
{
"type": "mcp",
"server_label": "dmcp",
"server_description": "A Dungeons and Dragons MCP server to assist with dice rolling.",
"server_url": "https://dmcp-server.deno.dev/mcp",
"require_approval": "never",
},
],
input="Roll 2d4+1",
)which is the current supported shape in our gateway so far. since our current agentic loop not handling the approval path we can support that later.
The shape of require_approval : { "never": {"tool_names":[]}} is specific to filtering approval.
with example:
from openai import OpenAI
client = OpenAI()
resp = client.responses.create(
model="gpt-5.6",
tools=[
{
"type": "mcp",
"server_label": "deepwiki",
"server_url": "https://mcp.deepwiki.com/mcp",
"require_approval": {
"never": {
"tool_names": ["ask_question", "read_wiki_structure"]
}
}
},
],
input="What transport protocols does the 2025-03-26 version of the MCP spec (modelcontextprotocol/modelcontextprotocol) support?",
)will make this comment to an issue for this to add for future support.
|
|
||
| The rest of the stack (`McpHandler`, `build_mcp_registry`, dispatch loop) is unchanged. Stdio servers are just another entry in the pool. | ||
| The internal function-tool representation is an implementation detail and must not leak as a public function call. | ||
| The current gateway work implements the `mcp_call` lifecycle; emitting the separate `mcp_list_tools` discovery |
There was a problem hiding this comment.
This says "the current gateway work implements the mcp_call lifecycle," but the code ships mcp_tool_call (output.rs:423, response.mcp_tool_call.* events). codex-integration.md already says mcp_tool_call, so just this file is out of sync — flip to mcp_tool_call + a note that the mcp_call rename lands in #139, so the doc matches the wire.
There was a problem hiding this comment.
added in the document that the design is expected to be completed in sequential PRs.
| ## Recorder coverage | ||
|
|
||
| **Future:** once both PRs are stable, `execute()` and `execute_with_mcp()` are unified into `execute_loop()`. | ||
| `crates/agentic-server-core/tests/cassettes/record_mcp_cassettes.sh` records matching native-MCP scenarios against the |
There was a problem hiding this comment.
record_mcp_cassettes.sh is referenced here but was removed in this PR, and mcp_tool_test.rs is param/registry deserialization only (no streaming/public-exposure assertions). Trim this section to what's tested, or mark it planned for #139.
There was a problem hiding this comment.
removed this entire part as there is no recorded coverage as off now.
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
Signed-off-by: maral <maralbahari.98@gmail.com>
This was addressed during the initial MCP support work, with subsequent SSRF hardening adding an explicit hostname allowlist as the operator trust boundary by fransico; restricting resolved address ranges would be a separate policy change because private MCP servers may be intentional. This PR does not change that behavior from
If is start touching renaming
removed this and some other tests that were referring to |
ashwing
left a comment
ashwing
left a comment
There was a problem hiding this comment.
Thanks — this all looks reasonable, approving.
- SSRF: fair — it predates this PR and restricting resolved address ranges is a separate policy call (private MCP servers are a legitimate use). Agreed it's out of scope here.
- Field rename / doc: makes sense to keep the
mcp_callfield+type rename in #139 and have the design doc describe the end-state contract with the sequential-PR note — reads clearly now. require_approval: the scalar"never"path is the right v1, and #145 covers the object/approval-filtering form. One breadcrumb for #145: the object form currently failsMcpToolParamdeserialization and falls through theother -> Unknownarm, so the server is dropped silently rather than erroring — worth making that a clear "approval not yet supported" error there rather than a silent drop.- Fixture cleanup +
tools/listerror propagation look good.
LGTM.
|
@franciscojavierarceo addressed the comments. Could you please check again. thanks |
3 participants
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-compatibletype: "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:
read_mcp_resourcefunction bridge andresources/readexecution.read_mcp_resourceas an ordinary client-executed function.type: "mcp"declarations usingserver_labeland connection information without a client-supplied tool name.tools/list.allowed_toolsfiltering to discovered tools.ToolRegistry.server_label.tools/call.This PR intentionally retains the existing
mcp_tool_callpublic output item and streaming lifecycle. A follow-up PR will align public output items and SSE events with OpenAI'smcp_callcontract.Important Note:
MCP name collision handling: MCP declarations no longer expose the non-standard
mcp.namefield. 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 --workspacecargo test --workspacecargo clippy --all-targets -- -D warningscargo fmt -- --check