Support compartment - #1073
Support compartment#1073TingtingZhou7 wants to merge 5 commits into
Conversation
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis PR introduces comprehensive support for image generation tool calls across the MCP, protocol, and model gateway layers. It adds new configuration types, response formats, event handlers, and tool execution logic to enable end-to-end image generation functionality. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces support for a built-in image generation tool across the MCP and model gateway crates. It adds the necessary data structures, response formats, and streaming events to handle image generation calls, including logic to extract base64 image data and metadata from tool results. A significant addition is the compact_tool_output_for_model_context utility, which ensures that large binary image payloads are replaced with a minimal summary before being fed back into the model's conversation history. Furthermore, the PR implements header forwarding for SSE and streamable MCP transports and allows request-level tool overrides to be merged into tool-call arguments. I have no feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5830bda787
ℹ️ 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".
| let payload = Self::image_payload_from_wrapped_content(result) | ||
| .unwrap_or_else(|| Value::String(Self::flatten_mcp_output(result))); |
There was a problem hiding this comment.
Parse SSE-wrapped image tool output before fallback
This fallback runs whenever image_payload_from_wrapped_content returns None, but that helper only handles object-shaped result.content payloads. If the MCP response arrives as SSE text (for example event: message\ndata: {...}), the parser misses it and this branch stores the entire SSE frame as image_generation_call.result, so clients receive protocol framing text instead of the actual image/error payload.
Useful? React with 👍 / 👎.
| let is_error = is_image_generation_error(output); | ||
| let note = if is_error { | ||
| extract_image_generation_fallback_text(output).unwrap_or_default() | ||
| } else { | ||
| "Succsally generate the image".to_string() |
There was a problem hiding this comment.
Treat non-wrapper image tool errors as failed in context
This logic only considers result.isError, but tool failures in the main execution path are also emitted as { "error": ... } (for example from execute_tool_resolved on call failures). In those cases is_error is false here, so failed image calls are summarized as status: completed with a success note, which pollutes the tool-loop conversation state and can cause the next model turn to assume success instead of recovering.
Useful? React with 👍 / 👎.
| inputs.push(McpServerInput { | ||
| label: server_name, | ||
| url: Some(url), | ||
| authorization: token, | ||
| headers, |
There was a problem hiding this comment.
Avoid adding both dynamic and static bindings for builtins
When forwarded headers are configured, this block adds a dynamic binding for the builtin server (URL-keyed), but the same builtin type is still passed to ensure_mcp_servers, which also appends the static binding (name-keyed). Because deduping is by server_key, both bindings survive, exposing duplicate tool entries and allowing one route that bypasses forwarded request headers, leading to inconsistent auth behavior depending on which duplicate tool is selected.
Useful? React with 👍 / 👎.
Description
Problem
Solution
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit