fix(mcp): re-add image generation tool routing - #1091
TingtingZhou7 wants to merge 15 commits into
Conversation
github-actions
Bot
added
mcp
|
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:
📝 WalkthroughWalkthroughAdds image-generation as a built-in tool across MCP: new enums/types, transformer handling and helpers, protocol events, router integration, streaming/tool-loop changes, and compacted tool-output logic plus tests and a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ToolLoop as Tool Loop\n(model_gateway/src/.../tool_loop.rs)
participant Model as External Model
participant Transform as Transformer\n(crates/mcp/src/transformer.rs)
participant OutputCtx as Output Context\n(model_gateway/src/routers/tool_output_context.rs)
Client->>ToolLoop: execute_streaming_tool_calls(ResponsesRequest)
ToolLoop->>ToolLoop: parse & apply request tool overrides
ToolLoop->>Model: call image generation tool (args JSON)
Model-->>ToolLoop: returns JSON result (wrapped result or error)
ToolLoop->>Transform: is_image_generation_error(result)
Transform-->>ToolLoop: error? and fallback text (if any)
ToolLoop->>OutputCtx: compact_tool_output_for_model_context(format, result)
OutputCtx-->>ToolLoop: compact JSON {tool,status,note}
ToolLoop->>Client: emit ImageGenerationCall events (IN_PROGRESS / updates / COMPLETED)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
TingtingZhou7
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 a built-in image generation tool for the MCP framework, including new protocol definitions, transformation logic, and streaming event support. It introduces a mechanism to override tool arguments from the request level and a utility to compact image generation outputs for model context to avoid large binary payloads. Review feedback identifies an opportunity to simplify repetitive result extraction logic in the transformer and recommends parameterizing a hardcoded tool name in the output compaction utility to improve robustness.
![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: 7ec7ac76c0
ℹ️ 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".
TingtingZhou7
changed the title
![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)
model_gateway/src/routers/openai/responses/utils.rs (1)
206-233: 🧹 Nitpick | 🔵 TrivialLGTM! Consider updating the docstring.
The
ImageGenerationvariant handling follows the same serialization pattern asWebSearchPreviewandCodeInterpreter. The docstring at lines 207-209 lists "web_search_preview, and code_interpreter" but doesn't mentionimage_generation.📝 Optional: Update docstring to include image_generation
/// Convert a single ResponseTool back to its original JSON representation. /// -/// Handles MCP tools (with server metadata), web_search_preview, and code_interpreter. +/// Handles MCP tools (with server metadata), web_search_preview, code_interpreter, and image_generation. /// Returns None for function tools and other types that don't need restoration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/openai/responses/utils.rs` around lines 206 - 233, Update the function docstring for response_tool_to_value to list ImageGeneration alongside WebSearchPreview and CodeInterpreter: modify the comment lines describing handled variants (currently mentioning "web_search_preview, and code_interpreter") to also include "image_generation" so the documentation matches the match arms for ResponseTool::ImageGeneration and others in this function.model_gateway/src/routers/openai/mcp/tool_loop.rs (1)
689-702:⚠️ Potential issue | 🟠 MajorThread the builtin
response_formatthrough themax_tool_callsearly-exit path.Line 689 now resolves
ResponseFormat::ImageGenerationCall, butbuild_incomplete_response()still reconstructs pending calls as genericmcp_callitems. If the loop stops onmax_tool_calls, an unexecuted image-generation call is returned with the wrong discriminator/id shape instead ofimage_generation_call/ig_*.🤖 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 689 - 702, The early-exit on max_tool_calls currently computes response_format via session.tool_response_format(&call.name) but then calls build_incomplete_response(...) which reconstructs pending calls as generic mcp_call items; thread the computed response_format into the early-exit call so build_incomplete_response can produce the correct discriminator/ID shape (e.g., return image_generation_call / ig_* when response_format == ResponseFormat::ImageGenerationCall). Update the invocation of build_incomplete_response to accept the response_format (or add an overload) and ensure build_incomplete_response uses that value when reconstructing pending calls for the max_tool_calls path.
🤖 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/openai/mcp/tool_loop.rs`:
- Around line 279-295: The current tools.iter().find_map(...) in tool_loop.rs
silently picks the first ResponseTool::ImageGeneration config, making behavior
order-dependent; update validation to reject duplicate image-generation tool
definitions instead of allowing multiple or, alternatively, make merge semantics
deterministic. Concretely, add a uniqueness check in validate_response_tools()
to error when more than one ResponseTool::ImageGeneration is present (or, if you
prefer merging, implement a deterministic merge routine and replace the find_map
use with an explicit aggregate that sorts/merges deterministically). Reference
ResponseTool::ImageGeneration, tools.iter(), and validate_response_tools() when
making the change so the PR rejects duplicate image_generation configs during
validation or clearly implements deterministic merging.
---
Outside diff comments:
In `@model_gateway/src/routers/openai/mcp/tool_loop.rs`:
- Around line 689-702: The early-exit on max_tool_calls currently computes
response_format via session.tool_response_format(&call.name) but then calls
build_incomplete_response(...) which reconstructs pending calls as generic
mcp_call items; thread the computed response_format into the early-exit call so
build_incomplete_response can produce the correct discriminator/ID shape (e.g.,
return image_generation_call / ig_* when response_format ==
ResponseFormat::ImageGenerationCall). Update the invocation of
build_incomplete_response to accept the response_format (or add an overload) and
ensure build_incomplete_response uses that value when reconstructing pending
calls for the max_tool_calls path.
In `@model_gateway/src/routers/openai/responses/utils.rs`:
- Around line 206-233: Update the function docstring for response_tool_to_value
to list ImageGeneration alongside WebSearchPreview and CodeInterpreter: modify
the comment lines describing handled variants (currently mentioning
"web_search_preview, and code_interpreter") to also include "image_generation"
so the documentation matches the match arms for ResponseTool::ImageGeneration
and others in this function.
🪄 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: 10da9d3b-f378-473a-bb85-fc965b2008b8
📒 Files selected for processing (14)
.gitignorecrates/mcp/src/core/config.rscrates/mcp/src/lib.rscrates/mcp/src/transform/mod.rscrates/mcp/src/transform/transformer.rscrates/mcp/src/transform/types.rscrates/protocols/src/event_types.rscrates/protocols/src/responses.rsmodel_gateway/src/routers/mcp_utils.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rsmodel_gateway/src/routers/openai/responses/streaming.rsmodel_gateway/src/routers/openai/responses/utils.rsmodel_gateway/src/routers/tool_output_context.rs
|
Seems lint is failing. @TingtingZhou7 Can you take a look? |
|
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:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tool_output_context.rs`:
- Around line 7-10: Update the doc comment in tool_output_context.rs to match
the implementation: state that ResponseFormat::ImageGenerationCall is compacted
into a minimal summary that DOES include a status field (mention the status
field explicitly), and clarify that other formats generally return
output.to_string() unchanged except that JSON strings are special-cased (they
are attempted to be parsed/deserialized and compacted). Reference
ResponseFormat::ImageGenerationCall, the status field, output.to_string(), and
the JSON special-case logic so the comment accurately describes current
behavior.
- Around line 23-25: Summary: The JSON uses an undefined identifier `tool_name`,
causing a compile error; replace it with the image generation tool identifier.
Locate the json! block that constructs `summary` in tool_output_context.rs (the
line creating {"tool": tool_name, ...}) and change `tool_name` to the correct
image-generation constant/variable used elsewhere in the module (e.g.,
IMAGE_GENERATION_TOOL_ID or the module's image tool identifier), ensuring the
identifier is in scope and imported if necessary so the JSON uses a defined
symbol.
🪄 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: 84c7479f-b9fa-4779-b1f1-078b257f47d7
📒 Files selected for processing (1)
model_gateway/src/routers/tool_output_context.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
model_gateway/src/routers/tool_output_context.rs (2)
23-25:⚠️ Potential issue | 🔴 Critical
tool_nameis undefined and will fail lint/compile.There is no in-scope binding for
tool_namein this file. Use the concrete image-generation tool identifier or a shared constant instead.Proposed fix
let summary = json!({ - "tool": tool_name, + "tool": "image_generation_call", "status": if is_error { "failed" } else { "completed" }, "note": note });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/tool_output_context.rs` around lines 23 - 25, The JSON construction for `summary` references an undefined variable `tool_name`, which will fail compilation; replace `tool_name` with the correct concrete identifier or shared constant for the image-generation tool (e.g., use the existing image generator constant or literal name) wherever `summary` is built in the function that contains the `let summary = json!({ ... })` expression so the binding is in-scope and the code compiles.
7-10:⚠️ Potential issue | 🟡 MinorUpdate the doc comment to match the code.
The comment says the image summary has “no payload/status” and that other formats return
output.to_string()unchanged, but the implementation includes astatusfield and special-casesValue::Stringby returning the raw string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/tool_output_context.rs` around lines 7 - 10, Update the doc comment above the compacting logic to accurately describe the current behavior: state that ResponseFormat::ImageGenerationCall is compacted into a minimal fixed summary that includes a status field (but omits the large binary payload), and that other formats are not strictly no-ops — Value::String is returned as the raw string instead of being wrapped, while other non-string formats fall back to output.to_string(). Mention ResponseFormat::ImageGenerationCall, the presence of the status field, and the special-case handling of Value::String to make the comment match the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model_gateway/src/routers/tool_output_context.rs`:
- Around line 23-25: The JSON construction for `summary` references an undefined
variable `tool_name`, which will fail compilation; replace `tool_name` with the
correct concrete identifier or shared constant for the image-generation tool
(e.g., use the existing image generator constant or literal name) wherever
`summary` is built in the function that contains the `let summary = json!({ ...
})` expression so the binding is in-scope and the code compiles.
- Around line 7-10: Update the doc comment above the compacting logic to
accurately describe the current behavior: state that
ResponseFormat::ImageGenerationCall is compacted into a minimal fixed summary
that includes a status field (but omits the large binary payload), and that
other formats are not strictly no-ops — Value::String is returned as the raw
string instead of being wrapped, while other non-string formats fall back to
output.to_string(). Mention ResponseFormat::ImageGenerationCall, the presence of
the status field, and the special-case handling of Value::String to make the
comment match the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ce38e72e-3421-49d0-a1a0-9828cbc0cab9
📒 Files selected for processing (1)
model_gateway/src/routers/tool_output_context.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4f638a375
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 329d2bfb2a
ℹ️ 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".
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/grpc/harmony/builder.rs (1)
418-435:⚠️ Potential issue | 🟡 MinorKeep
image_generationclassified consistently.Adding
ResponseTool::ImageGeneration(_)totool_typeswithout also updatingBUILTIN_TOOLSmakeshas_custom_tools(&tool_types)returntruefor image-generation-only requests. That flips this path into the custom-tool flow and can emit an empty developer message for what should be a builtin tool. Either remove this mapping while gRPC rejects the tool, or add"image_generation"to the builtin classification used here.🛠️ Minimal fix
-const BUILTIN_TOOLS: &[&str] = &["web_search_preview", "code_interpreter", "container"]; +const BUILTIN_TOOLS: &[&str] = &[ + "web_search_preview", + "code_interpreter", + "image_generation", + "container", +];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/builder.rs` around lines 418 - 435, The tool classification adds ResponseTool::ImageGeneration to the tool_types vector which isn't listed in BUILTIN_TOOLS, causing has_custom_tools(&tool_types) to treat image-generation-only requests as custom; either remove the ResponseTool::ImageGeneration branch from the mapping in the block that builds tool_types or add the string "image_generation" to the BUILTIN_TOOLS set used by has_custom_tools so that image generation is treated as a builtin; update the mapping or BUILTIN_TOOLS accordingly and keep the references: tool_types, ResponseTool::ImageGeneration(_), has_custom_tools, BUILTIN_TOOLS, and with_custom_tools.
♻️ Duplicate comments (1)
model_gateway/src/routers/tool_output_context.rs (1)
7-10:⚠️ Potential issue | 🟡 MinorDoc comment is still out of sync.
Lines 7-10 still say the compact summary has “no payload/status” and that other formats return
output.to_string()unchanged, but Line 25 emitsstatusand Lines 31-33 preserve raw strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/tool_output_context.rs` around lines 7 - 10, Update the module doc comment to match the actual behavior: state that ResponseFormat::ImageGenerationCall is compacted into a minimal summary that includes status (not "no payload/status"), and clarify that other formats preserve raw strings (not always returning output.to_string()). Reference the ResponseFormat::ImageGenerationCall term and the compacting logic that emits status and the branches that preserve raw strings so the comment accurately reflects lines emitting status and the raw-string-preserving branches.
🤖 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/grpc/harmony/builder.rs`:
- Around line 418-435: The tool classification adds
ResponseTool::ImageGeneration to the tool_types vector which isn't listed in
BUILTIN_TOOLS, causing has_custom_tools(&tool_types) to treat
image-generation-only requests as custom; either remove the
ResponseTool::ImageGeneration branch from the mapping in the block that builds
tool_types or add the string "image_generation" to the BUILTIN_TOOLS set used by
has_custom_tools so that image generation is treated as a builtin; update the
mapping or BUILTIN_TOOLS accordingly and keep the references: tool_types,
ResponseTool::ImageGeneration(_), has_custom_tools, BUILTIN_TOOLS, and
with_custom_tools.
---
Duplicate comments:
In `@model_gateway/src/routers/tool_output_context.rs`:
- Around line 7-10: Update the module doc comment to match the actual behavior:
state that ResponseFormat::ImageGenerationCall is compacted into a minimal
summary that includes status (not "no payload/status"), and clarify that other
formats preserve raw strings (not always returning output.to_string()).
Reference the ResponseFormat::ImageGenerationCall term and the compacting logic
that emits status and the branches that preserve raw strings so the comment
accurately reflects lines emitting status and the raw-string-preserving
branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42911d29-bb39-4440-af62-a8b5896c1eea
📒 Files selected for processing (5)
crates/mcp/src/transform/transformer.rsmodel_gateway/src/routers/grpc/common/responses/streaming.rsmodel_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/tool_output_context.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: affd5110d5
ℹ️ 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".
|
| ResponseTool::Function(_) => "function", | ||
| ResponseTool::WebSearchPreview(_) => "web_search_preview", | ||
| ResponseTool::CodeInterpreter(_) => "code_interpreter", | ||
| ResponseTool::ImageGeneration(_) => "image_generation", |
There was a problem hiding this comment.
I'm not following the design here. It seems Harmony still accepts the image generation tool call?
There was a problem hiding this comment.
Nvm. I saw this is just the builder
There was a problem hiding this comment.
Yes. Because harmony and openai share this ResponseTool file. And if i delete it, complition will fail. I have added it to make build succeed. But if you check the changes in "Utils", I have reject the request with image_generation_tool.
There was a problem hiding this comment.
Please address this in a follow up PR. If Harmony should not support image generation, there are cleaner compile-safe options than “pretend it is a supported Harmony tool type here, then reject later in utils”.
For example,
ResponseTool::ImageGeneration(_) => continue
Or we could add a image generation mcp server, even a fake one so we can test this can work on harmony or other models.
| let event_type = match response_format { | ||
| ResponseFormat::WebSearchCall => WebSearchCallEvent::IN_PROGRESS, | ||
| ResponseFormat::CodeInterpreterCall => CodeInterpreterCallEvent::IN_PROGRESS, | ||
| ResponseFormat::ImageGenerationCall => ImageGenerationCallEvent::IN_PROGRESS, |
There was a problem hiding this comment.
In theory, the code path here won't be triggered, as we are disabling the image_generation_tool and returning a 400. Is that right?
There was a problem hiding this comment.
Yes. User request are gated before this.
There was a problem hiding this comment.
Same here. We can address this in a follow up PR.
There was a problem hiding this comment.
It's hard to remove it since we use an exhaustive match. If we remove exhaustive match, do you see any concern here?
github-actions
Bot
removed
mesh
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: edd6146f66
ℹ️ 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 input = output.to_string(); | ||
| let result = if matches!(self, ResponseFormat::ImageGenerationCall) { |
There was a problem hiding this comment.
compact_tool_output_for_model_context serializes output to a string up front for debug logging, regardless of log level. This method is now called on every tool result, so large outputs (especially image/base64 payloads) incur extra CPU and allocation cost even when debug logs are disabled; for non-image formats it also ends up serializing the same value again for result in the else branch.
Useful? React with 👍 / 👎.
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75c3606fd8
ℹ️ 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".
| } | ||
| }; | ||
|
|
||
| apply_request_tool_overrides(&response_format, original_body, &mut arguments); |
There was a problem hiding this comment.
Applying apply_request_tool_overrides here mutates the arguments right before tool execution, but in the OpenAI streaming path the earlier response.mcp_call_arguments.* events are still derived from the model’s original call.arguments_buffer. When clients request image options like size/quality in tools, the stream can report one argument payload while the server executes another, which breaks replay/audit assumptions for streaming consumers and makes debugging tool behavior difficult.
Useful? React with 👍 / 👎.
…iers Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
|
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:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 679d912119
ℹ️ 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".
| } | ||
| }; | ||
|
|
||
| apply_request_tool_overrides(&response_format, original_body, &mut arguments); |
There was a problem hiding this comment.
execute_tool_loop now mutates tool arguments with apply_request_tool_overrides, but the PendingApproval path still calls build_mcp_approval_request_item with call.arguments (the pre-override JSON). In requests that set image-generation overrides and hit interactive MCP approval, the approval item can show stale parameters while the pending execution uses different arguments (pending.arguments_str), so users may approve a different call than what is displayed. The approval payload should be built from the effective overridden arguments.
Useful? React with 👍 / 👎.
Signed-off-by: TingtingZhou7 <zhoutt96@gmail.com>
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: 9041e61002
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea31e32781
ℹ️ 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".
| ResponseFormat::ImageGenerationCall => { | ||
| (ItemType::IMAGE_GENERATION_CALL, "ig_") | ||
| } |
There was a problem hiding this comment.
Adding ImageGenerationCall to the output_item type rewrite here makes response.output_item.added/done use ig_* IDs, but the same stream still rewrites function_call.arguments.done to mcp_call.arguments.done with mcp_* item IDs in apply_event_transformations_inplace. In streaming image-tool calls (OpenAI Responses + MCP routing), clients that correlate by item_id will see argument events on a different logical item than the emitted image call, which breaks replay/audit pipelines and can drop argument metadata for that tool call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9041e61002
ℹ️ 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: 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: f6c25b6042
ℹ️ 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: 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: 52cbda6c1d
ℹ️ 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: 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: 561a40f176
ℹ️ 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".
| content.iter().find_map(|item| { | ||
| item.as_object() | ||
| .filter(|o| o.get("type").and_then(|v| v.as_str()) == Some("text")) | ||
| .and_then(|o| o.get("text")) | ||
| .and_then(|v| v.as_str()) | ||
| .filter(|text| !text.trim().is_empty()) | ||
| .map(str::to_string) |
There was a problem hiding this comment.
extract_image_generation_fallback_text currently returns the first non-empty text entry, so when MCP returns multiple text blocks (for example a leading JSON metadata/progress block followed by a real failure string), the transformed image_generation_call.result captures the metadata instead of the actionable error. This misreports failures and can mislead retry/debug logic in clients that rely on the returned result text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
valid point. I need to test case when mcp return multiple results. Noted down and will follow up on this. Should be good for this pr since it's a p2
|
|
||
| const GRPC_RESPONSES_SUPPORTED_TOOL_KINDS: &[ResponseToolKind] = &[ | ||
| ResponseToolKind::Function, | ||
| ResponseToolKind::WebSearchPreview, |
There was a problem hiding this comment.
I should remove this also to make it exclusive match, so a new tool added later won't be missed here
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: 469d96cc15
ℹ️ 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".
3 participants
Description
Problem
Remove gRPC from https://github.com/lightseekorg/smg/pull/1057/changes#diff-477dfd1a461c069d89a1fc610c9eb9e8d6f98fc6924ebc8de092a8ceca00d007
Solution
Disable image generation tool in gRPC.
Changes
Disable image generation tool in gRPC.
Test Plan
Tested with gRPC and image-generation-tool is disabled now.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Behavioral