duplicate of https://github.com/lightseekorg/smg/pull/1057#960
duplicate of https://github.com/lightseekorg/smg/pull/1057#960TingtingZhou7 wants to merge 1 commit intolightseekorg:mainfrom
Conversation
TingtingZhou7
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
github-actions
bot
added
grpc
|
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 first-class image-generation tool support across protocols, gateway routing, MCP config/transformer, and streaming: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as ModelGateway
participant MCP
participant Transformer as ResponseTransformer
Client->>Gateway: Request with tool_choice "image_generation"
Gateway->>Gateway: recognize builtin tool -> route decision
Gateway->>MCP: Forward request to MCP image-generation server
MCP->>Transformer: Return ResponseFormat::ImageGenerationCall payload
Transformer->>Gateway: to_image_generation_call -> ImageGenerationCall output item
Gateway->>Client: Respond with `image_generation_call` output item
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
|
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:
|
![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 adds support for a new 'image_generation' tool across the protocol and gateway layers. I have noted that the new functionality lacks corresponding tests, which should be addressed to ensure reliability. Additionally, I recommend formatting the 'matches!' expression in 'model_gateway/src/routers/grpc/harmony/builder.rs' to span multiple lines for better consistency and readability.
| matches!( | ||
| self.tool_type.as_str(), | ||
| "web_search_preview" | "code_interpreter" | "container" | ||
| "web_search_preview" | "code_interpreter" | "image_generation" | "container" |
There was a problem hiding this comment.
For consistency with other multi-line matches! expressions in this pull request and to improve readability, consider formatting this by putting each pattern on its own line.
"web_search_preview"
| "code_interpreter"
| "image_generation"
| "container"
TingtingZhou7
force-pushed
the
tingzhou/add-img
branch
2 times, most recently
from
fb61255 to
3814457
Compare
![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: d6e92d7239
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.oca/custom_code_review_guidelines.txt:
- Around line 1-24: The "Sample guideline" content (the Java streams example /
"Use streams instead of a loop for better readability") appears unrelated to the
PR's image-generation scope; either remove this
.oca/custom_code_review_guidelines.txt change from the PR, or explicitly justify
its inclusion by updating the PR description and the file to state how these
review guidelines relate to the "Add Support for image generation tool" feature
(or move the guidelines to a separate documentation-only PR); ensure the file
text clearly references the image-generation context if kept.
- Line 22: Document that the example's use of Collection.toList() requires Java
16+ and either add a note stating the minimum Java version (Java 16) or change
the example to use Stream.collect(Collectors.toList()) for Java 8–15
compatibility; if you choose the latter, also note the mutability difference
(Collection.toList() returns an immutable List, while Collectors.toList()
returns a mutable list on older JDKs).
- Around line 9-22: The Java example in the template is inappropriate for this
Rust repo and contains API/type mismatches: replace the Java snippet
(references: List<Integer>, ArrayList, for (int number : numbers),
Arrays.stream(numbers), .toList()) with either a corrected Java variant that
preserves semantics (use numbers.stream() for collections and
collect(Collectors.toList()) if you need a mutable ArrayList) or—preferably—swap
the entire example for an equivalent Rust iterator sample using Vec and
iter().map(...).collect() so the example matches the repository language and
avoids the type and mutability problems.
In `@oracle-schema-config.local.yaml`:
- Line 1: The repo-wide config currently enables automatic DB DDL by setting
auto_migrate: true; change this key to auto_migrate: false in this committed
config so startup won't execute Oracle migrations by default, and instead enable
auto-migrate only in local/runtime overrides or explicit non-repo configs;
verify that the startup migration code paths (oracle.rs startup logic and
versioning.rs migration runner) will respect the config and that the default
safety check in schema.rs remains effective when the flag is false.
🪄 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: d029e5cb-9b7c-444d-95d1-137b236a3f9f
📒 Files selected for processing (6)
.oca/custom_code_review_guidelines.txtcrates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/openai/responses/utils.rsoracle-schema-config.local.yaml
| # ```java | ||
| # // Calculate squares of numbers | ||
| # List<Integer> squares = new ArrayList<>(); | ||
| # for (int number : numbers) { | ||
| # squares.add(number * number); | ||
| # } | ||
| # ``` | ||
| # Recommendation: | ||
| # | ||
| # ```java | ||
| # // Calculate squares of numbers | ||
| # List<Integer> squares = Arrays.stream(numbers) | ||
| # .map(n -> n * n) // Map each number to its square | ||
| # .toList(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check Java version and stream usage patterns
# Check if Java version is documented anywhere
rg -i 'java.*(version|[0-9]{2})' --glob '*.md' --glob '*.txt' --glob 'pom.xml' --glob 'build.gradle*' -C2
# Check for existing .toList() usage (Java 16+ API)
rg '\.toList\(\)' --type=java -C2
# Check for Collectors.toList() usage (pre-Java 16 pattern)
rg 'Collectors\.toList\(\)' --type=java -C2Repository: lightseekorg/smg
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Explore repository structure and primary language
echo "=== Repository Structure ==="
git ls-files | head -30
echo -e "\n=== File Extensions ==="
git ls-files | sed 's/.*\.//' | sort | uniq -c | sort -rn | head -20
echo -e "\n=== Read the file being reviewed ==="
if [ -f .oca/custom_code_review_guidelines.txt ]; then
cat -n .oca/custom_code_review_guidelines.txt
else
echo "File not found"
fiRepository: lightseekorg/smg
Length of output: 2505
Java example in template has type and API compatibility issues; consider replacing with Rust example matching repository language.
This is a template guideline file in a Rust repository. While the Java code example (lines 9-22) is intended as a sample pattern, it contains technical issues that would mislead developers:
- Type mismatch: The original loop
for (int number : numbers)works with anyIterable, but the recommendedArrays.stream(numbers)requires an array type. Ifnumbersis aList<Integer>, the recommendation won't compile. - Mutability difference:
ArrayList(original) is mutable;.toList()(recommended, Java 16+) returns an immutable list—a significant behavioral change. - Language scope: A Rust project's review guidelines should ideally feature Rust examples, not Java, to match contributor context.
Consider either fixing the Java example to use numbers.stream() if it's a collection, or replacing it with a Rust iterator example that's more relevant to the project.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.oca/custom_code_review_guidelines.txt around lines 9 - 22, The Java example
in the template is inappropriate for this Rust repo and contains API/type
mismatches: replace the Java snippet (references: List<Integer>, ArrayList, for
(int number : numbers), Arrays.stream(numbers), .toList()) with either a
corrected Java variant that preserves semantics (use numbers.stream() for
collections and collect(Collectors.toList()) if you need a mutable ArrayList)
or—preferably—swap the entire example for an equivalent Rust iterator sample
using Vec and iter().map(...).collect() so the example matches the repository
language and avoids the type and mutability problems.
| # // Calculate squares of numbers | ||
| # List<Integer> squares = Arrays.stream(numbers) | ||
| # .map(n -> n * n) // Map each number to its square | ||
| # .toList(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What Java version introduced the Stream.toList() method and what are the differences between toList() and collect(Collectors.toList())?
💡 Result:
The Stream.toList method was introduced in Java 16. Key differences between Stream.toList and collect(Collectors.toList) are: - Mutability: toList returns an immutable List (throws UnsupportedOperationException on mutator methods). collect(Collectors.toList) returns a mutable List (typically ArrayList, modifiable). - Syntax: toList is more concise (no need for collect or Collectors import). collect requires stream.collect(Collectors.toList). - Null handling: Both allow null elements. - Type inference: collect(Collectors.toList) is more flexible with supertypes (e.g., Stream.collect(toList) to List); toList infers exact stream type, may need casts or explicit typing. - Performance: toList is slightly more optimized (direct array deposition, no Collector overhead). - Availability: toList since Java 16; Collectors.toList since Java 8. Note: In Java 21+, Collectors.toList was updated to return immutable lists matching toList behavior.
Citations:
- 1: https://bugs.openjdk.org/browse/JDK-8257870
- 2: https://stackoverflow.com/questions/65969919/differences-of-java-16s-stream-tolist-and-stream-collectcollectors-tolist
- 3: https://bugs.openjdk.org/browse/JDK-8256441
- 4: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/stream/Stream.html
- 5: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/stream/Collectors.html
Document Java version requirement for .toList().
The .toList() method was introduced in Java 16. If the project targets earlier Java versions, this example will not compile. For Java 8-15 compatibility, use .collect(Collectors.toList()) instead. Note: .toList() returns an immutable List, while Collectors.toList() returns a mutable one (though in Java 21+, Collectors.toList() also returns an immutable list).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.oca/custom_code_review_guidelines.txt at line 22, Document that the
example's use of Collection.toList() requires Java 16+ and either add a note
stating the minimum Java version (Java 16) or change the example to use
Stream.collect(Collectors.toList()) for Java 8–15 compatibility; if you choose
the latter, also note the mutability difference (Collection.toList() returns an
immutable List, while Collectors.toList() returns a mutable list on older JDKs).
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/grpc/common/responses/utils.rs (1)
25-29: 🧹 Nitpick | 🔵 TrivialUpdate docstring to include
image_generation.The docstring mentions
web_search_previewandcode_interpreteras builtin tool types but doesn't includeimage_generation, which is now also handled.📝 Suggested fix
/// Ensure MCP connection succeeds if MCP tools or builtin tools are declared /// -/// Checks if request declares MCP tools or builtin tool types (web_search_preview, -/// code_interpreter), and if so, validates that the MCP clients can be created +/// Checks if request declares MCP tools or builtin tool types (web_search_preview, +/// code_interpreter, image_generation), and if so, validates that the MCP clients can be created /// and connected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/common/responses/utils.rs` around lines 25 - 29, Update the docstring for the MCP connection check to also mention the builtin tool type image_generation; locate the comment/block above the function that "Ensure MCP connection succeeds if MCP tools or builtin tools are declared" (the docstring referencing builtin tool types web_search_preview and code_interpreter) and add image_generation to that list so the documentation matches current behavior.model_gateway/src/routers/openai/responses/utils.rs (1)
206-209: 🧹 Nitpick | 🔵 TrivialUpdate docstring to include
image_generation.The function docstring mentions "MCP tools", "web_search_preview", and "code_interpreter" but should also include
image_generationsince it's now handled.📝 Suggested fix
/// 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 - 209, Update the function docstring that begins "Convert a single ResponseTool back to its original JSON representation." to also mention image_generation alongside MCP tools, web_search_preview, and code_interpreter; explicitly state that image_generation is handled and that the function still returns None for function tools and other types that don't need restoration so readers see image_generation is supported.
🤖 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/protocols/src/responses.rs`:
- Around line 39-41: Add the missing ImageGeneration tool into the MCP routing
surfaces: update the BuiltinToolType enum in crates/mcp/src/core/config.rs to
include ImageGeneration and add the corresponding ImageGenerationCall variant to
ResponseFormatConfig; then modify extract_builtin_types() in
model_gateway/src/routers/mcp_utils.rs so it extracts ImageGeneration (and
ensure FileSearch is also extracted there), and update the response_format()
implementation for BuiltinToolType to handle the new ImageGeneration variant
(return the proper ResponseFormatConfig::ImageGenerationCall). Use the enum and
function names BuiltinToolType, ResponseFormatConfig, extract_builtin_types, and
response_format to locate the spots to change.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/common/responses/utils.rs`:
- Around line 25-29: Update the docstring for the MCP connection check to also
mention the builtin tool type image_generation; locate the comment/block above
the function that "Ensure MCP connection succeeds if MCP tools or builtin tools
are declared" (the docstring referencing builtin tool types web_search_preview
and code_interpreter) and add image_generation to that list so the documentation
matches current behavior.
In `@model_gateway/src/routers/openai/responses/utils.rs`:
- Around line 206-209: Update the function docstring that begins "Convert a
single ResponseTool back to its original JSON representation." to also mention
image_generation alongside MCP tools, web_search_preview, and code_interpreter;
explicitly state that image_generation is handled and that the function still
returns None for function tools and other types that don't need restoration so
readers see image_generation is supported.
🪄 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: 5dfee3c3-70d3-451b-a54f-8bb74542dcde
📒 Files selected for processing (6)
.oca/custom_code_review_guidelines.txtcrates/protocols/src/responses.rsmodel_gateway/src/routers/grpc/common/responses/utils.rsmodel_gateway/src/routers/grpc/harmony/builder.rsmodel_gateway/src/routers/openai/responses/utils.rsoracle-schema-config.local.yaml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec689199c9
ℹ️ 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.
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 `@crates/mcp/src/transform/transformer.rs`:
- Around line 396-420: Add additional unit tests for
ResponseTransformer::transform covering the edge cases for
ResponseFormat::ImageGenerationCall: one test that passes a direct JSON string
(to exercise the fallback path that treats the whole payload as a string), one
test that passes a non-string/non-object JSON value (to exercise the branch that
calls to_string() on the payload), and one test that asserts error payload
handling (if ResponseTransformer returns an error variant or Error payload
field) to verify the transformer produces the expected ImageGenerationCall
status and result; keep the same test structure as
test_image_generation_transform but vary the input payloads and expected id
("ig_req-..."), status, and result assertions.
- Around line 106-124: The to_image_generation_call function currently always
returns ResponseOutputItem::ImageGenerationCall with
ImageGenerationCallStatus::Completed and Some(result); update it to inspect the
MCP result for error/partial indicators (e.g., presence of "error" key, an
explicit "status" field like "failed"/"incomplete"/"generating", or non-2xx
error payload shapes) and map those to the appropriate ImageGenerationCallStatus
variants (InProgress/Generating/Incomplete/Failed) from the protocol, and set
result to None or include the error message only for Failed/Incomplete as
appropriate; locate and change logic in to_image_generation_call and the
construction of ResponseOutputItem::ImageGenerationCall (id, status, result) so
downstream consumers see correct status instead of always Completed.
🪄 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: ca785e4b-e333-4866-b2ad-c5634359f518
📒 Files selected for processing (4)
crates/mcp/src/core/config.rscrates/mcp/src/transform/transformer.rscrates/mcp/src/transform/types.rsmodel_gateway/src/routers/mcp_utils.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e03b4b7c90
ℹ️ 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 (2)
model_gateway/src/routers/grpc/common/responses/streaming.rs (2)
476-484: 🧹 Nitpick | 🔵 TrivialConsider defining a constant for the event type string.
The other variants use constants (e.g.,
WebSearchCallEvent::IN_PROGRESS). For consistency and maintainability, consider adding a similar constant likeImageGenerationCallEvent::IN_PROGRESSin the event_types module rather than using a hardcoded string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/common/responses/streaming.rs` around lines 476 - 484, The match arm for ResponseFormat::ImageGenerationCall is using a hardcoded string; instead add a constant like ImageGenerationCallEvent::IN_PROGRESS in the event_types module and use that constant here. Update the event_types module to define ImageGenerationCallEvent::IN_PROGRESS (matching the naming/pattern of WebSearchCallEvent::IN_PROGRESS), then change the match to return ImageGenerationCallEvent::IN_PROGRESS so all ResponseFormat variants use the same constant pattern and maintain consistency with emit_tool_event.
125-158:⚠️ Potential issue | 🔴 CriticalField mismatch:
ImageGenerationCallusesidandresult, notcall_idandoutput.Per
crates/protocols/src/responses.rs(Context snippet 2),ResponseOutputItem::ImageGenerationCallhas fieldsid,status, andresult. However, this function:
- Looks up
call_id(line 141) which doesn't exist onImageGenerationCall- Sets
output(line 147) instead ofresultAs a result,
ImageGenerationCallitems will pass theis_tool_callcheck but never match, silently failing to update.🐛 Proposed fix to handle ImageGenerationCall field differences
let is_tool_call = matches!( item_type, Some("mcp_call") | Some("web_search_call") | Some("code_interpreter_call") | Some("file_search_call") | Some("image_generation_call") ); - if is_tool_call - && item_data.get("call_id").and_then(|c| c.as_str()) - == Some(&tool_result.call_id) - { + // ImageGenerationCall uses `id` instead of `call_id` + let call_id_field = if item_type == Some("image_generation_call") { + "id" + } else { + "call_id" + }; + if is_tool_call + && item_data.get(call_id_field).and_then(|c| c.as_str()) + == Some(&tool_result.call_id) + { // Add output field let output_str = serde_json::to_string(&tool_result.output) .unwrap_or_else(|_| "{}".to_string()); - item_data["output"] = json!(output_str); + // ImageGenerationCall uses `result` instead of `output` + let output_field = if item_type == Some("image_generation_call") { + "result" + } else { + "output" + }; + item_data[output_field] = json!(output_str);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/common/responses/streaming.rs` around lines 125 - 158, The update_mcp_call_outputs function currently matches tool call items by looking for "call_id" and writes an "output" field, which fails for ImageGenerationCall items that use "id" and "result"; modify update_mcp_call_outputs (and the loop that checks item_type == "image_generation_call") to treat image_generation_call specially: compare item_data.get("id").and_then(|v| v.as_str()) to tool_result.call_id (instead of "call_id") and write the tool_result.output into item_data["result"] (preserving the same serialization strategy you use elsewhere), while keeping the existing behavior for other tool types that use "call_id" and "output"; ensure you still set item_data["status"] = "failed" when tool_result.is_error.
🤖 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/common/responses/streaming.rs`:
- Around line 476-484: The match arm for ResponseFormat::ImageGenerationCall is
using a hardcoded string; instead add a constant like
ImageGenerationCallEvent::IN_PROGRESS in the event_types module and use that
constant here. Update the event_types module to define
ImageGenerationCallEvent::IN_PROGRESS (matching the naming/pattern of
WebSearchCallEvent::IN_PROGRESS), then change the match to return
ImageGenerationCallEvent::IN_PROGRESS so all ResponseFormat variants use the
same constant pattern and maintain consistency with emit_tool_event.
- Around line 125-158: The update_mcp_call_outputs function currently matches
tool call items by looking for "call_id" and writes an "output" field, which
fails for ImageGenerationCall items that use "id" and "result"; modify
update_mcp_call_outputs (and the loop that checks item_type ==
"image_generation_call") to treat image_generation_call specially: compare
item_data.get("id").and_then(|v| v.as_str()) to tool_result.call_id (instead of
"call_id") and write the tool_result.output into item_data["result"] (preserving
the same serialization strategy you use elsewhere), while keeping the existing
behavior for other tool types that use "call_id" and "output"; ensure you still
set item_data["status"] = "failed" when tool_result.is_error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4aa4c854-ad76-461e-acc3-b967a6e7aed9
📒 Files selected for processing (2)
model_gateway/src/routers/grpc/common/responses/streaming.rsmodel_gateway/src/routers/openai/mcp/tool_loop.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 648d3a748f
ℹ️ 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
TingtingZhou7
force-pushed
the
tingzhou/add-img
branch
from
4630856 to
bb24298
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ca57cc806
ℹ️ 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".
| ItemType::WEB_SEARCH_CALL => WebSearchCallEvent::COMPLETED, | ||
| ItemType::CODE_INTERPRETER_CALL => CodeInterpreterCallEvent::COMPLETED, | ||
| ItemType::FILE_SEARCH_CALL => FileSearchCallEvent::COMPLETED, | ||
| "image_generation_call" => ImageGenerationCallEvent::COMPLETED, |
There was a problem hiding this comment.
This new arm emits response.image_generation_call.completed, but in the same OpenAI streaming flow transform_streaming_event still maps non-web-search builtins to mcp_call with mcp_ IDs (model_gateway/src/routers/openai/responses/streaming.rs:146-149). For image-generation requests, clients will see response.output_item.added as mcp_call and then completion/done as image_generation_call for the same output index, which changes type/ID mid-stream and breaks consumers that correlate events by item identity.
Useful? React with 👍 / 👎.
| if item_type == Some("image_generation_call") { | ||
| let result_str = tool_result | ||
| .output |
There was a problem hiding this comment.
The new image-generation update path only extracts result from tool_result.output and never reads a returned status; afterwards status is only changed when tool_result.is_error is true. In the Harmony Responses streaming path (which calls this updater before response.completed), payloads such as {"status":"generating"} or {"status":"incomplete"} will be surfaced as completed calls, so clients lose the newly added status semantics and can treat unfinished generations as final.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d4662056a
ℹ️ 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".
| | Some("web_search_call") | ||
| | Some("code_interpreter_call") | ||
| | Some("file_search_call") | ||
| | Some("image_generation_call") |
There was a problem hiding this comment.
By including image_generation_call in this shared updater, image tool items now pass through the generic error-status logic below, which writes "failed" when tool_result.is_error is true. However ResponseOutputItem::ImageGenerationCall uses ImageGenerationCallStatus (in_progress|completed|incomplete) and does not accept failed, so failed image items become schema-invalid; in finalize() they fail serde_json::from_value(...).ok() and get dropped from the persisted/final response. This occurs in the gRPC Harmony streaming path whenever an MCP image tool returns an error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79070250c7
ℹ️ 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: 4a8cd870d8
ℹ️ 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".
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
3 similar comments
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
github-actions
bot
added
anthropic
TingtingZhou7
force-pushed
the
tingzhou/add-img
branch
from
07a3769 to
26a376a
Compare
github-actions
bot
removed
documentation
TingtingZhou7
force-pushed
the
tingzhou/add-img
branch
from
2633bf4 to
55942ab
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55942ab653
ℹ️ 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".
| pub enum ImageGenerationCallStatus { | ||
| InProgress, | ||
| Generating, | ||
| Completed, | ||
| Failed, | ||
| } |
There was a problem hiding this comment.
incomplete image status variant
ImageGenerationCallStatus does not include an Incomplete variant, so any image_generation_call payload with "status":"incomplete" cannot deserialize into ResponseOutputItem. In the streaming path this is particularly risky because ResponseStreamEventEmitter::finalize() deserializes output items with serde_json::from_value(...).ok(), so the image item can be silently dropped from the final response when that status appears. Adding the missing enum variant keeps image status round-trippable and prevents losing output items.
Useful? React with 👍 / 👎.
TingtingZhou7
changed the title
TingtingZhou7
changed the title
1 participant
Please refer to #1057