feat(mcp): forward OCI headers exclusively to MCP tool calls via JSON-RPC _meta#1415
Conversation
Signed-off-by: aravind <aravindh.raja@oracle.com>
Signed-off-by: aravind <aravindh.raja@oracle.com>
aravindh-raja
requested review from
CatherineSue,
key4ng,
slin1237,
zhaowenzi and
zhoug9127
as code owners
github-actions
Bot
added
mcp
📝 WalkthroughWalkthroughHTTP headers are extracted into a new MCP-specific set and threaded through MCP orchestration so per-request forwarded headers are sent in peer tool calls. OpenAI handlers now pass MCP-only headers into MCP sessions. FileSearch is recognized as a hosted built-in and file-search parsing constants were introduced. ChangesMCP Orchestration, Header Extraction & OpenAI handlers
FileSearch routing & parsing
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant GW as Gateway (header_utils)
participant Orch as MCP Orchestrator
participant Server as MCP Server
Client->>GW: HTTP request with headers
GW->>GW: extract_mcp_forward_headers(headers)
GW->>Orch: McpToolSession::new_with_headers(mcp_headers)
Orch->>Orch: approval flow / execute_tool_with_reconnect(forwarded_headers)
Orch->>Orch: execute_tool_impl(..., forwarded_headers)
Orch->>Orch: call_tool_on_peer(request, forwarded_headers)
alt forwarded_headers non-empty
Orch->>Server: send_request_with_option(JSON-RPC with _meta.extra_headers)
else forwarded_headers empty
Orch->>Server: peer.call_tool(request)
end
Server-->>Orch: CallToolResult
Orch-->>GW: Tool response
GW-->>Client: Final response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
aravindh-raja
changed the title
![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 implements header forwarding for MCP tool calls, specifically targeting OCI delegation tokens and compartment IDs. The changes include refactoring header extraction utilities to support MCP-specific headers, updating the orchestrator to pass these headers through the JSON-RPC _meta field, and adding support for the FileSearch built-in tool. Feedback is provided regarding a performance optimization in orchestrator.rs to avoid unnecessary string clones by using into_iter() when constructing the metadata map.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
crates/mcp/src/core/orchestrator.rs (1)
1484-1548:⚠️ Potential issue | 🟠 MajorRequest-scoped dynamic tools still bypass
_meta.extra_headers.
call_tool_on_peer()fixes the static/pool-backed execution paths, butMcpRequestContext::execute_dynamic_tool()in Line 2117 still callsclient.call_tool(request)directly. Any tool reached throughadd_dynamic_server()therefore skips this new header injection entirely, so OCI headers will disappear on that path. Please route that method throughcall_tool_on_peer()or a shared helper as well.Supporting change outside this selected range
- let result = client - .call_tool(request) - .await - .map_err(|e| McpError::ToolExecution(format!("MCP call failed: {e}")))?; + let result = self + .orchestrator + .call_tool_on_peer( + client.peer(), + entry.server_key(), + request, + &self.forwarded_headers, + ) + .await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/mcp/src/core/orchestrator.rs` around lines 1484 - 1548, execute_dynamic_tool currently calls client.call_tool(request) directly which bypasses the new _meta.extra_headers injection; update McpRequestContext::execute_dynamic_tool to route dynamic tool calls through the existing call_tool_on_peer (or extract the call logic into a shared helper) so the forwarded_headers map is passed and injected into Meta like in call_tool_on_peer; ensure you construct/obtain the same peer (&rmcp::service::Peer<RoleClient>) and server_key used for error mapping, pass the original request and forwarded_headers through, and replace direct client.call_tool(...) calls with call_tool_on_peer(peer, &server_key, request, &forwarded_headers) (or call the new shared helper) so OCI headers are preserved for add_dynamic_server paths.model_gateway/src/routers/common/header_utils.rs (1)
303-330:⚠️ Potential issue | 🟠 MajorDon't comma-join singleton auth headers.
This helper now applies CSV-style merging to every repeated header. That is fine for
tracestate, but it corrupts singleton credentials likeauthorization,x-smg-oci-delegation-token, andx-smg-oci-compartment-id: two header instances become one synthetic value that downstream auth code cannot interpret reliably. Please keep list semantics only for headers that actually allow it, and otherwise reject or pick a single value.Possible fix
fn extract_headers_matching( headers: Option<&HeaderMap>, predicate: fn(&str) -> bool, ) -> HashMap<String, String> { + fn supports_csv_join(name: &str) -> bool { + name.eq_ignore_ascii_case("tracestate") + } + let Some(headers) = headers else { return HashMap::new(); }; @@ - forwarded - .entry(name_str.to_string()) - .and_modify(|existing: &mut String| { - existing.push_str(", "); - existing.push_str(value); - }) - .or_insert_with(|| value.to_string()); + forwarded + .entry(name_str.to_string()) + .and_modify(|existing: &mut String| { + if supports_csv_join(name_str) { + existing.push_str(", "); + existing.push_str(value); + } + }) + .or_insert_with(|| value.to_string()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/common/header_utils.rs` around lines 303 - 330, The extract_headers_matching function is currently comma-joining every repeated header which breaks singleton auth headers; change the merging logic to only CSV-merge for a allowlist (e.g., "tracestate") and treat other headers (specifically "authorization", "x-smg-oci-delegation-token", "x-smg-oci-compartment-id") as singletons: when inserting, if the header is not in the CSV allowlist then keep the first value and ignore subsequent occurrences (do not push ", " + value) instead of concatenating; implement this by adding a small allowlist constant and branching inside the .entry(...).and_modify(...) / .or_insert_with(...) flow in extract_headers_matching to apply CSV merging only for allowlisted names and otherwise leave the existing value unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/mcp/src/core/orchestrator.rs`:
- Around line 1459-1462: The doc comment currently references the wrong JSON-RPC
field name; update the comment that mentions `_meta.forwarded_headers` to
instead document `_meta.extra_headers` so it matches the implementation that
serializes forwarded_headers into `_meta.extra_headers`; search for the comment
blocks that describe forwarded_headers (e.g., the comment above the
forwarded_headers handling) and change both occurrences to `_meta.extra_headers`
to keep docs consistent with the code.
---
Outside diff comments:
In `@crates/mcp/src/core/orchestrator.rs`:
- Around line 1484-1548: execute_dynamic_tool currently calls
client.call_tool(request) directly which bypasses the new _meta.extra_headers
injection; update McpRequestContext::execute_dynamic_tool to route dynamic tool
calls through the existing call_tool_on_peer (or extract the call logic into a
shared helper) so the forwarded_headers map is passed and injected into Meta
like in call_tool_on_peer; ensure you construct/obtain the same peer
(&rmcp::service::Peer<RoleClient>) and server_key used for error mapping, pass
the original request and forwarded_headers through, and replace direct
client.call_tool(...) calls with call_tool_on_peer(peer, &server_key, request,
&forwarded_headers) (or call the new shared helper) so OCI headers are preserved
for add_dynamic_server paths.
In `@model_gateway/src/routers/common/header_utils.rs`:
- Around line 303-330: The extract_headers_matching function is currently
comma-joining every repeated header which breaks singleton auth headers; change
the merging logic to only CSV-merge for a allowlist (e.g., "tracestate") and
treat other headers (specifically "authorization", "x-smg-oci-delegation-token",
"x-smg-oci-compartment-id") as singletons: when inserting, if the header is not
in the CSV allowlist then keep the first value and ignore subsequent occurrences
(do not push ", " + value) instead of concatenating; implement this by adding a
small allowlist constant and branching inside the .entry(...).and_modify(...) /
.or_insert_with(...) flow in extract_headers_matching to apply CSV merging only
for allowlisted names and otherwise leave the existing value unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5cb57901-f609-40c1-a4a6-bf8fbc6b10fb
📒 Files selected for processing (5)
crates/mcp/src/core/orchestrator.rsmodel_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/common/mcp_utils.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rs
Signed-off-by: aravind <aravindh.raja@oracle.com>
Signed-off-by: aravind <aravindh.raja@oracle.com>
|
Hi @aravindh-raja, 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
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)
crates/mcp/src/transform/transformer.rs (1)
243-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse file-search payloads from MCP text blocks.
call_result_to_json()feeds this transformer the MCPcontentarray. If a file-search server returns a plain JSON object through a text block ([{"type":"text","text":"{\"query\":...,\"results\":[...] }"}]),extract_file_results()at Line 567 only inspectsresult.as_object(), and the single-query fallback at Line 249 only reads a top-level object. That emits afile_search_callwith emptyresults/querieseven though the MCP payload contains hits. Reuseparse_text_block_payload()here the same way the web-search and image-generation paths already do.Also applies to: 563-599
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcp/src/transform/transformer.rs` around lines 243 - 257, The file-search branch is only inspecting result.as_object() and a single top-level object for queries, so it misses MCP text-block payloads; update the file-search path in call_result_to_json (the code building ResponseOutputItem::FileSearchCall) to reuse parse_text_block_payload() the same way web-search/image paths do: first call parse_text_block_payload(result) and if it yields an object/array use that parsed payload for extract_file_results() and for reading KEY_QUERY (fall back to parsed payload for single-query extraction), then proceed to build queries and results (using extract_file_results and the existing queries fallback) so text-block JSON payloads are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/routers/common/header_utils.rs`:
- Around line 303-312: Remove "traceparent" from the CSV_MERGE_HEADERS constant
so it is no longer treated as a comma-joinable header; update the
CSV_MERGE_HEADERS array to only include "tracestate" and ensure
is_csv_mergeable(name: &str) continues to use that constant so traceparent is
handled as a singleton (first-value-wins) like other single-value headers.
---
Outside diff comments:
In `@crates/mcp/src/transform/transformer.rs`:
- Around line 243-257: The file-search branch is only inspecting
result.as_object() and a single top-level object for queries, so it misses MCP
text-block payloads; update the file-search path in call_result_to_json (the
code building ResponseOutputItem::FileSearchCall) to reuse
parse_text_block_payload() the same way web-search/image paths do: first call
parse_text_block_payload(result) and if it yields an object/array use that
parsed payload for extract_file_results() and for reading KEY_QUERY (fall back
to parsed payload for single-query extraction), then proceed to build queries
and results (using extract_file_results and the existing queries fallback) so
text-block JSON payloads are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67a5f803-6a18-4e75-bf9f-01aba006b3a8
📒 Files selected for processing (5)
crates/mcp/src/core/orchestrator.rscrates/mcp/src/transform/transformer.rsmodel_gateway/src/routers/common/header_utils.rsmodel_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rs
| /// Headers where multiple values are comma-joined per RFC 9110. | ||
| /// All other headers are treated as singletons (first value wins). | ||
| const CSV_MERGE_HEADERS: &[&str] = &["tracestate", "traceparent"]; | ||
|
|
||
| /// Returns `true` if repeated values for `name` should be comma-joined. | ||
| fn is_csv_mergeable(name: &str) -> bool { | ||
| CSV_MERGE_HEADERS | ||
| .iter() | ||
| .any(|h| name.eq_ignore_ascii_case(h)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the W3C Trace Context specification allow multiple traceparentheaders or comma-joinedtraceparentvalues, or is onlytracestate allowed to be combined from multiple header fields?
💡 Result:
The W3C Trace Context specification does not allow multiple traceparent headers or comma-joined traceparent values. The traceparent header is designed as a single, fixed-format header field representing one incoming request's position in the trace graph [1][2][3]. The specification describes processing a single traceparent header: vendors check for "a traceparent header" and create a new one if absent or invalid [1][2]. It explicitly states the header name is "a single word" without delimiters, and always refers to it in singular form [1][3]. Multiple traceparent headers are treated as invalid by implementations (e.g., Elasticsearch rejects with 400, considering it malformed [4]; Nginx bugs produce comma-joined values incorrectly [5]), aligning with the spec's intent for exactly one valid traceparent per request. In contrast, tracestate is explicitly allowed to be sent or received as multiple header fields, which MUST be combined per RFC 9110 Section 5.3 (or RFC 7230 in older drafts) [1][2][3]. It SHOULD be a single field when possible but MAY be split, enabling combination from multiple sources. No provision exists in the spec (Level 1 Recommendation [2][6] or Level 2 Candidate Recommendation [1]) for multiple or comma-joined traceparent values; such cases lead to ignoring the header and starting a new trace [3].
Citations:
- 1: https://www.w3.org/TR/trace-context-2
- 2: https://w3.org/TR/trace-context-1
- 3: https://github.com/w3c/trace-context/blob/main/spec/20-http_request_header_format.md
- 4: Requests to Elasticsearch with two
traceparentheaders should yield400 Bad Requestelastic/elasticsearch#107338 - 5: Traceparent is incorrect when the incoming request contains large headers open-telemetry/opentelemetry-cpp-contrib#267
- 6: https://www.w3.org/TR/trace-context/
Remove traceparent from the CSV-merge headers list.
The W3C Trace Context specification designates traceparent as a single, fixed-format header field that cannot be comma-joined across multiple header occurrences. Only tracestate is explicitly allowed to be combined from multiple header fields per RFC 9110. Including traceparent in CSV_MERGE_HEADERS (line 305) causes repeated traceparent values to be incorrectly merged into an invalid format like "a, b", which downstream trace processors will reject. Keep traceparent as a singleton (first value wins), treating it like other single-value headers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model_gateway/src/routers/common/header_utils.rs` around lines 303 - 312,
Remove "traceparent" from the CSV_MERGE_HEADERS constant so it is no longer
treated as a comma-joinable header; update the CSV_MERGE_HEADERS array to only
include "tracestate" and ensure is_csv_mergeable(name: &str) continues to use
that constant so traceparent is handled as a singleton (first-value-wins) like
other single-value headers.
2 participants
Summary
MCP tool calls previously had no mechanism to receive per-request headers like OCI delegation tokens from the incoming gateway request. This change introduces header forwarding to MCP tool calls by embedding custom headers in the JSON-RPC
_meta.extra_headersfield of thetools/callrequest.What changed
model_gateway/src/routers/common/header_utils.rsextract_mcp_forward_headers()— extracts only MCP-specific headers (x-smg-oci-delegation-token,x-smg-oci-compartment-id) from incoming requests.extract_headers_matching()helper used by bothextract_forwardable_request_headers()andextract_mcp_forward_headers().extract_forwardable_request_headers()no longer includes MCP-specific headers, keeping them out of LLM provider calls.model_gateway/src/routers/openai/responses/non_streaming.rsmodel_gateway/src/routers/openai/responses/streaming.rsextract_mcp_forward_headers()and merged into the forwarded headers map before passing toMcpToolSession.crates/mcp/src/core/orchestrator.rscall_tool_on_peer()embeds forwarded headers in_meta.extra_headersof the JSON-RPCtools/callrequest using rmcp'ssend_request_with_optionAPI.crates/mcp/src/core/handler.rs/crates/mcp/src/core/session.rsHandlerRequestContextandMcpToolSessioncarryforwarded_headersthrough the call chain from the router layer to the orchestrator.Why
OCI-backed MCP servers require per-request headers such as
x-smg-oci-delegation-tokenandx-smg-oci-compartment-idto authorize tool execution on behalf of the caller. Without this change, these headers were not forwarded to MCP tool calls and the MCP server had no way to receive them.These headers are specific to MCP tool calls and must not be sent to LLM providers (OpenAI, Anthropic, etc.), so they need a dedicated extraction path separate from the general header forwarding used for tracing, auth, and routing.
How
Headers flow through the system as follows:
x-smg-oci-delegation-tokenand/orx-smg-oci-compartment-idheaders.extract_mcp_forward_headers()to extract only MCP-specific headers, and merges them with the general forwarded headers.McpToolSession::new_with_headers(), which stores it on the session.McpRequestContextcarrying theforwarded_headers.call_tool_on_peer()checks for non-empty forwarded headers and, when present, constructs aPeerRequestOptionswithmeta: Some(Meta({ "extra_headers": { ... } })).peer.send_request_with_option()instead ofpeer.call_tool(), embedding the headers in the JSON-RPC_metafield of thetools/callrequest._meta.extra_headersin the protocol message.This approach keeps header forwarding at the JSON-RPC protocol level rather than the HTTP transport layer. Because the headers are part of the message payload, there is no shared mutable state, no race conditions, and concurrent tool calls to the same server execute without serialization.
Test plan
cargo test -p smg --lib header_utils— 23 unit tests pass, including:test_extract_mcp_forward_headers— verifies only OCI headers are returnedtest_extract_mcp_forward_headers_empty_when_none— verifies empty/None handlingtest_extract_forwardable_excludes_mcp_only_headers— verifies OCI headers are excluded from general extractiontest_is_mcp_forward_header— verifies the MCP header filtertest_should_forward_request_header_blocked— verifies OCI headers are not in the general forwarding allowlistcargo test -p smg-mcp— 201 unit tests pass, includingtest_session_preserves_forwarded_headers_in_request_context_meta.extra_headersx-smg-oci-*headersVerification
Summary by CodeRabbit
New Features
Enhancements
Tests