Repository navigation
Support passing dynamic header from response api to mcp servers - #1082
TingtingZhou7 wants to merge 20 commits into
Conversation
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
…ls output Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 24 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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 |
|
Hi @TingtingZhou7, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 768f121b41
ℹ️ 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".
| .is_some_and(|cfg| { | ||
| cfg.response_format != ResponseFormatConfig::Passthrough |
There was a problem hiding this comment.
Treat any tool config as explicit builtin override
In the new dynamic-connection path, this check only considers a tool override “explicit” when response_format != Passthrough, so a config that intentionally sets Passthrough gets overwritten by builtin_type.response_format(). That changes the emitted output item type for builtin-routed tools (for example from mcp_call to web_search_call) and breaks parity with the static path (apply_builtin_response_format), which preserves any explicit tool entry regardless of value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request introduces support for forwarding request headers to MCP servers and adds configuration for tool response formats and argument mapping. It also updates the MCP orchestrator to handle built-in tool routing and refactors the tool loop to remove redundant MCP list tool injections. I have identified two high-severity issues: the dynamic server configuration logic potentially misapplying built-in tool capabilities, and the removal of MCP list tool injections in the non-streaming response path, which breaks consistency.
I am having trouble creating individual review comments. Click here to see my feedback.
crates/mcp/src/core/orchestrator.rs (1607-1650)
The implementation for applying tool configurations to dynamic servers appears to treat them similarly to built-in tools. However, in the MCP orchestrator, built-in tool functionality is restricted to static servers, and dynamic servers are not intended to be configured this way. Please ensure this logic does not introduce built-in tool capabilities for dynamic servers.
References
- In the MCP orchestrator, dynamic servers are not intended to be configured as built-in tools; built-in tool functionality is restricted to static servers.
model_gateway/src/routers/openai/mcp/tool_loop.rs (833-839)
Similar to the issue in inject_mcp_metadata_streaming, removing the mcp_list_tools injection here breaks the non-streaming path. Since build_incomplete_response is called for non-streaming responses when the tool loop terminates early, the mcp_list_tools items should be included in the incomplete response to ensure consistency between execution paths.
References
- To maintain consistency, changes to a feature on one execution path (e.g., streaming) should align with its behavior on related paths (e.g., non-streaming).
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
…in routing Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
… path Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
|
Hi @TingtingZhou7, 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 |
|
Hi @TingtingZhou7, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae482d3dda
ℹ️ 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".
| inputs.push(McpServerInput { | ||
| label: server_name, | ||
| url: Some(url), | ||
| authorization: token, | ||
| headers, |
There was a problem hiding this comment.
Key forwarded-header MCP calls by full pool key
This new builtin path creates dynamic MCP inputs with per-request forwarded headers, but downstream execution still addresses dynamic servers by URL-only server_key. When two requests hit the same MCP URL with different forwarded header values, they produce distinct pooled connections but tool execution can select the wrong one via URL lookup, causing calls to run under another request’s header context. In practice this can misroute compartment-scoped calls and break tenant/request isolation for header-authenticated MCP backends.
Useful? React with 👍 / 👎.
| if !builtin_types.is_empty() { | ||
| for builtin_type in &builtin_types { | ||
| let Some((server_name, _, _, server_cfg)) = | ||
| mcp_orchestrator.find_builtin_server(*builtin_type) | ||
| else { |
There was a problem hiding this comment.
Skip static builtin fallback when dynamic header route exists
After adding dynamic builtin inputs for resolved forwarded headers, the same builtin type is still processed by static fallback logic, so a single builtin server can be represented twice (URL key + static name). That exposes duplicate function tools for the same underlying builtin and allows execution to land on the static binding that lacks forwarded headers, which is exactly the failure mode this change is trying to avoid for header-required tools.
Useful? React with 👍 / 👎.
|
duplicate of #1084 |
duplicate of #1084