feat(anthropic): add X-SMG-MCP header for MCP passthrough - #517
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces ad-hoc MCP e2e test scaffolding with data-driven McpTestConfig-based tests (SMG-handled and passthrough modes); adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AnthropicRouter
participant MCP_Server
participant UpstreamModel
Client->>AnthropicRouter: POST request (may include `x-smg-mcp` header + MCP toolset)
alt header == "enabled" AND MCP toolset present
AnthropicRouter->>MCP_Server: construct MCP server (config includes "type":"url")
AnthropicRouter->>UpstreamModel: forward request referencing MCP_Server
UpstreamModel-->>AnthropicRouter: model response / stream
MCP_Server-->>AnthropicRouter: MCP events/responses (streaming or non-streaming)
AnthropicRouter-->>Client: aggregated response/events
else header missing/disabled OR no MCP toolset
AnthropicRouter->>UpstreamModel: forward request without MCP servers
UpstreamModel-->>AnthropicRouter: model response
AnthropicRouter-->>Client: model response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Summary of ChangesHello @key4ng, 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 enhances the MCP (Multi-Cloud Proxy) passthrough functionality by introducing support for the 'X-SMG-MCP' header, which dictates whether the SMG intercepts or forwards MCP fields to the Anthropic backend. To validate this behavior, new end-to-end tests have been implemented for both non-streaming and streaming request scenarios. 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
|
github-actions
Bot
added
tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the MCP functionality by making it opt-in via an x-smg-mcp header. When the header is absent, MCP-related fields are passed through to the Anthropic backend. The changes in the Rust router correctly implement this conditional logic. The PR also adds comprehensive e2e tests for both streaming and non-streaming passthrough scenarios. While the new tests are functionally correct and provide good coverage, they introduce significant code duplication with existing tests. I've left a comment suggesting a refactor to improve maintainability.
key4ng
changed the title
key4ng
marked this pull request as ready for review
February 24, 2026 02:51
key4ng
requested review from
CatherineSue,
XinyueZhang369 and
slin1237
as code owners
February 24, 2026 02:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/anthropic/router.rs (2)
128-138: 🧹 Nitpick | 🔵 TrivialAdd a log when an MCP-toolset request enters passthrough mode
When
smg_mcp_enabledisfalsebut the request hasmcp_toolsettools, the existinginfo!at line 133 logsmcp = falsewith no distinction from a plain non-MCP request. A brief log in theelsebranch would make passthrough-mode routing visible in production traces.💬 Suggested addition
} else { + if request.has_mcp_toolset() { + info!("MCP: x-smg-mcp header absent; forwarding mcp_toolset request to Anthropic backend"); + } None };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/anthropic/router.rs` around lines 128 - 138, Add an explicit log when a request with mcp_toolset tools is routed in passthrough mode: inside the branch where smg_mcp_enabled is false but request.mcp_toolset is present, emit an info! log (similar to the existing info! that logs model, streaming, mcp) indicating that the request is entering MCP passthrough mode; reference the same context variables (model_id, is_streaming, request.mcp_toolset, and mcp_servers) so the log appears alongside the existing "Processing Messages API request" entry and makes passthrough routing visible in production traces.
128-138: 🧹 Nitpick | 🔵 TrivialNo log when an MCP-toolset request is entering passthrough mode
When
smg_mcp_enabledisfalseand the request carriesmcp_toolsettools, the existinginfo!at line 133 only reportsmcp = false— indistinguishable from a plain non-MCP request. Adding a trace-level log in the else branch would make passthrough routing visible in production traces without adding noise to the happy path.💬 Suggested addition
} else { + if request.has_mcp_toolset() { + info!("MCP passthrough: x-smg-mcp header absent, forwarding mcp_toolset request to Anthropic backend"); + } None };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/anthropic/router.rs` around lines 128 - 138, The info! currently logs mcp = false and thus hides when an MCP-toolset request is being routed in passthrough mode; update the code so that when smg_mcp_enabled is false but the incoming request contains mcp_toolset (i.e. the branch that sets mcp_servers to None), emit a trace!-level log indicating "MCP toolset detected; entering passthrough mode" (or similar) including model_id and whether request.stream (is_streaming) to make passthrough routing visible; keep the existing info! block unchanged and add the trace! call in the else branch that sets mcp_servers = None so it only fires for passthrough cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/messages/test_mcp_tool.py`:
- Around line 39-47: SMG_HANDLED McpTestConfig currently defaults server_url to
localhost so tests like test_mcp_non_streaming and test_mcp_streaming try to
connect in CI; update the test parametrization to skip the SMG_HANDLED path when
no external MCP server is intended by wrapping that param with
pytest.mark.skipif(not os.environ.get("MCP_SERVER_URL"), reason="MCP_SERVER_URL
not set") or change SMG_HANDLED to set
server_url=os.environ.get("MCP_SERVER_URL") (no default) and add a
pytest.skip/skipif guard in the parametrize or test so both
test_mcp_non_streaming and test_mcp_streaming will be skipped unless
MCP_SERVER_URL is provided.
- Around line 39-47: SMG_HANDLED currently defaults server_url to localhost so
tests fail when the local MCP server is down; add a skip guard: define a
module-level _smg_server_available (e.g., bool(os.environ.get("MCP_SERVER_URL"))
or a light connectivity check) alongside the McpTestConfig definitions, then
apply pytest.mark.skipif(not _smg_server_available and mcp_mode ==
"smg_handled", reason="MCP_SERVER_URL not set; skipping smg_handled tests") to
the test class or the parametrized test functions that use SMG_HANDLED so
smg_handled variants are cleanly skipped when the server is unavailable.
- Around line 50-55: The PASSTHROUGH McpTestConfig currently points to an
external uncontrolled server_url ("https://dmcp-server.deno.dev/sse"), causing
flaky CI; update tests using PASSTHROUGH to not call that external endpoint by
default: either add a custom pytest marker (e.g., `@pytest.mark.external`) to all
tests parametrized with PASSTHROUGH and configure pytest.ini to exclude that
marker in CI, or replace PASSTHROUGH.server_url with a locally hosted/test
fixture SSE MCP endpoint (or a mock SSE server) and wire the fixture into the
test harness so CI runs deterministically; update references to PASSTHROUGH and
any test parametrization so the external calls are only run when the marker is
explicitly requested.
- Around line 50-55: The PASSTHROUGH McpTestConfig uses a public server_url
("https://dmcp-server.deno.dev/sse") which introduces flaky CI failures; update
the test to avoid calling that external service by marking PASSTHROUGH with a
skip/marker (e.g., add `@pytest.mark.skipif` or `@pytest.mark.external` on the
parametrized variant that references PASSTHROUGH) driven by an environment flag
(e.g., RUN_EXTERNAL_TESTS) so it is excluded in default CI, or replace
PASSTHROUGH with a local DMCP test fixture endpoint under test control (create a
local/mock server used by the same PASSTHROUGH symbol) and ensure the test
harness/CI config excludes or enables the external marker accordingly.
In `@model_gateway/src/routers/anthropic/router.rs`:
- Line 91: smg_mcp_enabled is currently a presence-only check using
headers.and_then(|h| h.get("x-smg-mcp")).is_some(); add an inline comment next
to the smg_mcp_enabled declaration (referencing the smg_mcp_enabled variable and
the get("x-smg-mcp") call) that explicitly states this header is treated as a
presence flag (any value, including "false"/"disabled", enables SMG MCP) and, if
you want value-based behavior instead, suggest parsing the header value (e.g.,
checking for "true"/"1"/"enabled") rather than is_some().
- Line 91: The current check (let smg_mcp_enabled = headers.and_then(|h|
h.get("x-smg-mcp")).is_some();) treats any presence of the header as enabling
SMG MCP (so values like "false" or "disabled" still enable it); fix this by
reading and normalizing the header value instead of only testing presence:
replace the expression using headers.and_then(|h| h.get("x-smg-mcp")) to extract
a string (to_str().ok()), lowercase it, and match against an explicit allow-list
(e.g., "1","true","enabled","on") to set smg_mcp_enabled = true, treat known
deny values ("0","false","disabled","off","") as false, and default to false on
parse errors; alternatively, if presence-only semantics are intentional, add a
clear inline comment above the smg_mcp_enabled binding documenting that any
presence (regardless of value) enables SMG orchestration.
---
Outside diff comments:
In `@model_gateway/src/routers/anthropic/router.rs`:
- Around line 128-138: Add an explicit log when a request with mcp_toolset tools
is routed in passthrough mode: inside the branch where smg_mcp_enabled is false
but request.mcp_toolset is present, emit an info! log (similar to the existing
info! that logs model, streaming, mcp) indicating that the request is entering
MCP passthrough mode; reference the same context variables (model_id,
is_streaming, request.mcp_toolset, and mcp_servers) so the log appears alongside
the existing "Processing Messages API request" entry and makes passthrough
routing visible in production traces.
- Around line 128-138: The info! currently logs mcp = false and thus hides when
an MCP-toolset request is being routed in passthrough mode; update the code so
that when smg_mcp_enabled is false but the incoming request contains mcp_toolset
(i.e. the branch that sets mcp_servers to None), emit a trace!-level log
indicating "MCP toolset detected; entering passthrough mode" (or similar)
including model_id and whether request.stream (is_streaming) to make passthrough
routing visible; keep the existing info! block unchanged and add the trace! call
in the else branch that sets mcp_servers = None so it only fires for passthrough
cases.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
e2e_test/messages/test_mcp_tool.pymodel_gateway/src/routers/anthropic/router.rsprotocols/src/messages.rs
slin1237
requested changes
Feb 24, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/anthropic/router.rs`:
- Around line 91-96: The header-check for x-smg-mcp (building smg_mcp_enabled)
is case- and whitespace-sensitive; normalize the header value before matching by
converting to lowercase and trimming whitespace so values like "True", " enabled
", or "1" still enable MCP. Locate the smg_mcp_enabled computation (the headers
-> and_then -> to_str().ok() chain) and replace the predicate with a
normalization step (trim and to_ascii_lowercase) and then match against
"enabled" | "true" | "1"; keep the existing conditional that checks
request.has_mcp_toolset() and only enable mcp_servers when both are true.
… header and implementing non-streaming and streaming tests. Update MCP_EXTRA_HEADERS to include "x-smg-mcp" and validate tool usage in both test scenarios. Signed-off-by: key4ng <rukeyang@gmail.com>
…t value "url" This update introduces a new field, server_type, to the McpServerConfig struct, ensuring it defaults to "url". This enhancement improves the configuration clarity for MCP server types. Signed-off-by: [Your Name] [Your Email] Signed-off-by: key4ng <rukeyang@gmail.com>
…larity This update simplifies the assertion statements in the MCP passthrough tests by removing unnecessary line breaks, enhancing readability without altering functionality. Signed-off-by: key4ng <rukeyang@gmail.com>
This update refactors the MCP tool test suite by introducing a new McpTestConfig dataclass for better configuration management. It also streamlines the assertion logic for non-streaming responses, improving clarity and maintainability. The documentation has been updated to reflect the changes in server handling modes. Signed-off-by: key4ng <rukeyang@gmail.com>
This update refines the MCP tool test assertions by consolidating multiline statements into single lines for improved readability. The changes enhance the clarity of the assertions without affecting the functionality of the tests. Signed-off-by: key4ng <rukeyang@gmail.com>
This update improves the handling of the X-SMG-MCP header by ensuring it checks for specific string values ("enabled", "true", "1") after converting the header value to a string. This change enhances the clarity and robustness of the MCP toolset request validation.
Signed-off-by: key4ng <rukeyang@gmail.com>
…lidation Signed-off-by: key4ng <rukeyang@gmail.com>
key4ng
force-pushed
the
keyang/ant-passthru
branch
from
February 24, 2026 06:18
f3eab1c to
512ede5
Compare
This update simplifies the MCP server handling logic by directly integrating the check for the X-SMG-MCP header within the conditional statement. This change enhances code clarity and maintains the functionality of the MCP toolset request validation. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
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/header_utils.rs (1)
228-305: 🧹 Nitpick | 🔵 TrivialMissing unit tests for
is_smg_mcp_enabled.The test module covers
extract_routing_key,extract_target_worker, andshould_forward_request_headerthoroughly, butis_smg_mcp_enabledhas no test coverage. Consider adding tests for the accepted values, rejected values (e.g.,"false","disabled", empty string),Noneheaders, and missing header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/header_utils.rs` around lines 228 - 305, Add unit tests for is_smg_mcp_enabled: create tests that pass a HeaderMap with "x-smg-mcp-enabled" set to accepted true-like values (e.g., "true","1","enabled") and assert Some(true), tests with rejected values ("false","disabled","", "0") assert Some(false), a test with the header absent and with None headers asserting None; reference the function is_smg_mcp_enabled to locate where to call it and mirror the style of existing tests (using HeaderMap::new(), headers.insert(... .parse().unwrap()), and assert_eq!).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/header_utils.rs`:
- Around line 27-34: The check in is_smg_mcp_enabled incorrectly uses matches!
for exact, case-sensitive matching; change the final predicate to perform ASCII
case-insensitive comparisons on the header string (use
v.eq_ignore_ascii_case("enabled") || v.eq_ignore_ascii_case("true") ||
v.eq_ignore_ascii_case("1")) while keeping the existing header extraction via
headers.and_then(|h| h.get(&HEADER_MCP)).and_then(|v|
v.to_str().ok()).is_some_and(...) so the function uses eq_ignore_ascii_case
against HEADER_MCP's value.
---
Outside diff comments:
In `@model_gateway/src/routers/header_utils.rs`:
- Around line 228-305: Add unit tests for is_smg_mcp_enabled: create tests that
pass a HeaderMap with "x-smg-mcp-enabled" set to accepted true-like values
(e.g., "true","1","enabled") and assert Some(true), tests with rejected values
("false","disabled","", "0") assert Some(false), a test with the header absent
and with None headers asserting None; reference the function is_smg_mcp_enabled
to locate where to call it and mirror the style of existing tests (using
HeaderMap::new(), headers.insert(... .parse().unwrap()), and assert_eq!).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
e2e_test/messages/test_mcp_tool.pymodel_gateway/src/routers/anthropic/router.rsmodel_gateway/src/routers/header_utils.rsprotocols/src/messages.rs
…or 'enabled' header value This change modifies the is_smg_mcp_enabled function to specifically check for the 'enabled' value in the X-SMG-MCP header, simplifying the validation logic and improving clarity. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/header_utils.rs`:
- Around line 27-33: Add unit tests for the public function is_smg_mcp_enabled
to match coverage of the other public helpers (extract_routing_key,
extract_target_worker, should_forward_request_header): create tests that assert
true when the header X-SMG-MCP is present with values "enabled" in different
casings (e.g., "ENABLED", "Enabled"), and assert false for missing header, empty
value, and other values (e.g., "disabled"). Place tests in the same test module
pattern used for the other functions in header_utils.rs and use HeaderMap to
construct headers to exercise Option<&HeaderMap> inputs and ensure behavior
matches existing helpers.
slin1237
approved these changes
Feb 24, 2026
…er and simplify configurations This update introduces a new test marker for external dependencies in the MCP tool tests, allowing for better categorization of tests that require third-party services. Additionally, the configuration management is simplified by removing the unnecessary MCP_CONFIGS dictionary, streamlining the test setup for both SMG-handled and passthrough modes. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/messages/test_mcp_tool.py`:
- Around line 116-177: The test currently silently skips empty reassembled JSON
in assert_streaming_mcp_response (variable full_json built from
input_json_deltas_by_index) which can hide dropped fragments; update
assert_streaming_mcp_response to log a warning when full_json == "" (include idx
and the fragments list from input_json_deltas_by_index[idx]) so empty inputs are
visible during debugging while still allowing legitimate empty tool inputs;
locate this in assert_streaming_mcp_response (and use the module logger already
used at the end) and emit logger.warning with clear context instead of silently
continuing.
…Rust CI workflow This update introduces a test filter in the Rust CI workflow to exclude tests marked as external, enhancing the focus on internal test execution. This change aims to streamline the testing process and improve efficiency in the CI pipeline. Signed-off-by: key4ng <rukeyang@gmail.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem
SMG automatically intercepts and handles MCP orchestration whenever mcp_toolset tools are present in the Anthropic Messages API request body. There is no way for clients to pass MCP fields (mcp_servers, mcp_toolset) through to the Anthropic backend for native handling via anthropic-beta: mcp-client-2025-11-20
Solution
Introduce a X-SMG-MCP header to control MCP behavior. When present, SMG handles MCP orchestration (existing behavior). When absent, the request passes through to the Anthropic backend untouched, enabling Anthropic's native MCP support.
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Tests
Chores