refactor(mcp): simplify ensure_request_mcp_client and remove McpLoopConfig - #368
Conversation
Change prepare_mcp_tools_as_functions to take &McpToolSession instead of (&Arc<McpOrchestrator>, &[String]), eliminating redundant server_keys extraction at call sites since the session already caches this data.
…onfig Drop the redundant Arc<McpOrchestrator> from ensure_request_mcp_client's return type (callers already have it) and delete McpLoopConfig — callers now use DEFAULT_MAX_ITERATIONS directly and pass mcp_servers as a plain Vec.
Summary of ChangesHello @CatherineSue, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the MCP (Multi-Cloud Proxy) session setup within the OpenAI path to eliminate unnecessary indirection and simplify the API. The primary goal is to make the code cleaner and more direct by removing a redundant Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR refactors MCP (Model Control Protocol) architecture by removing the public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the MCP session setup, simplifying it by changing ensure_request_mcp_client to return only mcp_servers and removing the McpLoopConfig wrapper, which cleans up the code and reduces indirection. While this is a solid improvement, it introduces a significant security risk: the gateway allows users to specify arbitrary MCP server URLs without sufficient validation, leading to a potential Server-Side Request Forgery (SSRF) vulnerability. As this is a cross-cutting security concern requiring design decisions, it should be addressed in a dedicated pull request rather than being patched within this feature-specific PR.
…onfig (#368) Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
Description
Problem
MCP session setup in the OpenAI path had unnecessary indirection:
ensure_request_mcp_clientreturned a redundantArc<McpOrchestrator>clone that callers already had.McpLoopConfigwas a thin wrapper aroundmax_iterations+mcp_serverswith no real value.Solution
ensure_request_mcp_clientto returnOption<Vec<(String, String)>>— callers already hold the orchestrator.McpLoopConfigstruct andimpl Default— callers referenceDEFAULT_MAX_ITERATIONSdirectly and passmcp_serversas a plainVec.Changes
mcp_utils.rs: Simplified return type, removedMcpLoopConfigopenai/responses/non_streaming.rs: Use simplifiedensure_request_mcp_client, createMcpToolSessiondirectlyopenai/responses/streaming.rs: ReplaceMcpLoopConfigwith directmcp_servers+DEFAULT_MAX_ITERATIONSopenai/responses/mcp.rs: Removeconfigparam fromexecute_tool_loop, useDEFAULT_MAX_ITERATIONSgrpc/common/responses/utils.rs: Updateensure_mcp_connectionfor new return typeTest Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit