feat(mcp): route shell as built-in MCP tool (PR 1 of 2)#1424
feat(mcp): route shell as built-in MCP tool (PR 1 of 2)#1424RohanSogani wants to merge 3 commits into
Conversation
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
RohanSogani
requested review from
CatherineSue,
key4ng,
slin1237,
zhaowenzi and
zhoug9127
as code owners
github-actions
Bot
added
mcp
📝 WalkthroughWalkthroughAdds a new routed built-in tool type Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MG as ModelGateway
participant TF as Transformer
participant MCP as MCPServer
participant Tool as ShellTool
Client->>MG: request triggers tool call (shell_call)
MG->>MCP: route to routed builtin (builtin_type = Shell)
MCP->>TF: emit response with ResponseFormat::ShellCall
TF->>TF: parse shell action JSON, normalize id
TF->>MG: produce ResponseOutputItem::ShellCall (completed)
MG->>Client: stream final CALL_COMPLETED event (type="shell_call", id prefixed "sc")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Shell built-in tool type, updating the core configuration, session management, and routing utilities in the model gateway. Feedback suggests aligning the Shell response format with the protocol specification by using a dedicated variant instead of Passthrough. Additionally, several missing tool variants were identified: ImageGeneration should be added to the session's built-in tool list, and FileSearch needs to be included in the routing and extraction logic within mcp_utils.rs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/common/mcp_utils.rs (1)
187-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
file_searchis still omitted from builtin routing/extraction match arms.
ResponseTool::FileSearch(_)is missing in bothcollect_builtin_routingandextract_builtin_types, so requests using{"type":"file_search"}won’t enter MCP builtin routing through these helpers.🔧 Proposed fix
@@ let builtin_type = match tool { ResponseTool::WebSearchPreview(_) => BuiltinToolType::WebSearchPreview, ResponseTool::CodeInterpreter(_) => BuiltinToolType::CodeInterpreter, + ResponseTool::FileSearch(_) => BuiltinToolType::FileSearch, ResponseTool::ImageGeneration(_) => BuiltinToolType::ImageGeneration, ResponseTool::Shell(_) => BuiltinToolType::Shell, _ => continue, }; @@ .filter_map(|t| match t { ResponseTool::WebSearchPreview(_) => Some(BuiltinToolType::WebSearchPreview), ResponseTool::CodeInterpreter(_) => Some(BuiltinToolType::CodeInterpreter), + ResponseTool::FileSearch(_) => Some(BuiltinToolType::FileSearch), ResponseTool::ImageGeneration(_) => Some(BuiltinToolType::ImageGeneration), ResponseTool::Shell(_) => Some(BuiltinToolType::Shell), _ => None, })Also applies to: 229-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/common/mcp_utils.rs` around lines 187 - 193, Builtin file_search is omitted from the mapping: update both collect_builtin_routing and extract_builtin_types match expressions to include ResponseTool::FileSearch(_) => BuiltinToolType::FileSearch so requests with {"type":"file_search"} are handled; add the same arm in the other match (the one around lines 229-235) to keep both helpers consistent and ensure the FileSearch variant is routed/extracted like WebSearchPreview/CodeInterpreter/ImageGeneration/Shell.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/common/mcp_utils.rs`:
- Around line 187-193: Builtin file_search is omitted from the mapping: update
both collect_builtin_routing and extract_builtin_types match expressions to
include ResponseTool::FileSearch(_) => BuiltinToolType::FileSearch so requests
with {"type":"file_search"} are handled; add the same arm in the other match
(the one around lines 229-235) to keep both helpers consistent and ensure the
FileSearch variant is routed/extracted like
WebSearchPreview/CodeInterpreter/ImageGeneration/Shell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 510a7055-521f-4d8c-88b8-acb38d7c0c21
📒 Files selected for processing (3)
crates/mcp/src/core/config.rscrates/mcp/src/core/session.rsmodel_gateway/src/routers/common/mcp_utils.rs
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
github-actions
Bot
added
grpc
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b9cdbe4f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Rohan Sogani <ronsogani@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/openai/mcp/tool_loop.rs (1)
624-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't strip the shell
call_idin the non-streaming path.Unlike the other hosted formats,
shell_callkeeps both an output-itemidand the originalcall_id. By addingResponseFormat::ShellCallto this normalization branch,build_transformed_mcp_call_item()now feedsto_shell_call()the stripped item-id source, so a normal upstream pair likeid = "fc_123"/call_id = "call_123"is emitted asid = "sc_123"/call_id = "123". That breaks downstream correlation for follow-up shell items. Keep the rawcall_idfor shell and normalize only the outward-facingid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/mcp/tool_loop.rs` around lines 624 - 635, The function non_streaming_tool_item_id_source is incorrectly stripping prefixes for ResponseFormat::ShellCall; remove ShellCall from the branch that normalizes item IDs so shell call_ids remain raw. Update non_streaming_tool_item_id_source to treat ResponseFormat::ShellCall (and any shell-specific path) as a passthrough that returns item_id.to_string(), leaving the existing stripping behavior for WebSearchCall, CodeInterpreterCall, FileSearchCall, ImageGenerationCall, and ShellCall removed from that normalization branch; this preserves call_id when build_transformed_mcp_call_item() later calls to_shell_call().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 624-635: The function non_streaming_tool_item_id_source is
incorrectly stripping prefixes for ResponseFormat::ShellCall; remove ShellCall
from the branch that normalizes item IDs so shell call_ids remain raw. Update
non_streaming_tool_item_id_source to treat ResponseFormat::ShellCall (and any
shell-specific path) as a passthrough that returns item_id.to_string(), leaving
the existing stripping behavior for WebSearchCall, CodeInterpreterCall,
FileSearchCall, ImageGenerationCall, and ShellCall removed from that
normalization branch; this preserves call_id when
build_transformed_mcp_call_item() later calls to_shell_call().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0edb13b8-745f-4662-8c53-21781250d316
📒 Files selected for processing (7)
crates/mcp/src/core/config.rscrates/mcp/src/transform/transformer.rscrates/mcp/src/transform/types.rsmodel_gateway/src/routers/common/mcp_utils.rsmodel_gateway/src/routers/grpc/common/responses/streaming.rsmodel_gateway/src/routers/grpc/harmony/streaming.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rs
RohanSogani
changed the title
RohanSogani
changed the title
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Hi @RohanSogani, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
1 participant
Description
Problem
SMG can route several OpenAI Responses built-in tools to MCP servers via
builtin_type, includingweb_search_preview,code_interpreter,file_search, andimage_generation.The
shellResponses tool was already represented in the protocol layer, but it was not available as a routable MCP built-in type. As a result, a request declaring{"type":"shell"}could not be matchedto a configured MCP server using
builtin_type: shell.Additionally, mapping shell to generic MCP passthrough would expose the routed call as an
mcp_call, even though the Responses protocol has a nativeshell_calloutput item.Solution
Add
shellto the existing built-in MCP routing path, matching the pattern used for the other built-in tools.BuiltinToolType::Shellmaps to:shellshell_callResponseTool::Shell(_)This keeps the first upstream slice focused on shell routing and native
shell_callresponse-format alignment.Full hosted-shell output parity is intentionally left for a follow-up. OpenAI documents shell runs as paired output items:
shell_callfor the requested commands andshell_call_outputfor command outputand exit outcomes. That follow-up will add the broader one-tool-result-to-multiple-output-items plumbing needed for
shell_call_output.Reference: https://platform.openai.com/docs/guides/tools-shell#shell-output-in-responses
Changes
BuiltinToolType::Shell.shellbuilt-in type.ResponseFormatConfig::ShellCall.ResponseFormat::ShellCall.BuiltinToolType::ShelltoResponseFormatConfig::ShellCall.ResponseOutputItem::ShellCallinstead of generic passthrough.ResponseTool::Shell(_)in built-in MCP routing and extraction.BuiltinToolType::Shellin MCP session built-in binding detection.shell_callwithsc_item IDs instead of genericmcp_call.{"type":"shell"}routes to a configured MCP server withbuiltin_type: shell.ResponseFormat::ShellCallproducesResponseOutputItem::ShellCall.Test Plan
Repro from repo root:
Summary by CodeRabbit