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

[maralbahari]

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

[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.

[maralbahari]

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. the reason doc is changed to the whole overall picture of redesign that would be in #139 is to state the whole design for review to know what to expect while #138 is to just ease the shipment and PR review.

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

[franciscojavierarceo]

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.

[ashwing]

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.

[maralbahari]

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.

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 main

#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.

If is start touching renaming McptToolCall in this PR there will be many more files being touched. please let's go by the logical split this PR mainly removes mcp resources and fixes some of the design to add in the tool discovery in tool/executors.rs and tool/registy. the design doc is changed to tell the reader what Is the final overall mcp design contract not tide to #138 PR. I will add a note to the bottom the new design planned to ship in sequential with explanation of how the logics are separated.

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.

removed this and some other tests that were referring to read_mcp_resources.

[ashwing]

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_call field+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 fails McpToolParam deserialization and falls through the other -> Unknown arm, 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/list error propagation look good.

LGTM.

[maralbahari]

@franciscojavierarceo addressed the comments. Could you please check again. thanks

[franciscojavierarceo]

Thanks for the updates — the earlier points look addressed. I noticed a few follow-ups while checking the new version; leaving them inline.

[franciscojavierarceo]

I think this redaction can still be bypassed by the WebSocket generate: false path. That route forces store = true, then calls resp_handler.execute_turn() directly instead of going through persist_response(), so the MCP authorization and headers can still end up in effective_tools. Could we move the scrub down to where the response metadata is actually written and add a small WebSocket prewarm regression test?

[maralbahari]

Fixed by moving credential redaction into TryFrom<&ResponseMetadata> for String, the shared storage serialization boundary used by all persistence paths. Added a WebSocket generate: false regression test confirming MCP headers and authorization are not stored.

[franciscojavierarceo]

One more thing I noticed in the MCP call path:

[franciscojavierarceo]

Could we make the argument handling fail closed before calling the MCP server here? execute_tool_call() currently turns malformed JSON into None, which call_tool treats as a valid no-argument request. That means a tool with optional/default arguments could still run even though the model gave us invalid input. I think we should parse strictly, require an object, and return a ToolError without making the call if parsing fails.

[maralbahari]

Added parse_tool_arguments(), which is called first in execute_tool_call() to strictly parse and validate an argument object before any MCP server call is made.

[franciscojavierarceo]

And one small persistence cleanup:

[franciscojavierarceo]

Small follow-up: should we clear discovered_tools here too? Those server-provided schemas get persisted in effective_tools, but the field is skip_deserializing, so we throw them away when reading the record back. Stripping them here would avoid storing potentially large request-scoped data that we can't reuse.

[maralbahari]

Renamed redact_runtime_credentials() to sanitize_for_persistence(), which clears both MCP credentials and discovered_tools; it is called when ResponseMetadata is serialized for storage.