Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions model_gateway/src/routers/openai/mcp/tool_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,11 @@ fn send_tool_call_intermediate_event(
ResponseFormat::WebSearchCall => WebSearchCallEvent::SEARCHING,
ResponseFormat::CodeInterpreterCall => CodeInterpreterCallEvent::INTERPRETING,
ResponseFormat::FileSearchCall => FileSearchCallEvent::SEARCHING,
// stubbed: full event wiring (incl. partial_image payload events) is
// implemented in R6.2 for this router. Emitting `generating` as the
// generic intermediate keeps the shape consistent with the shared
// emitter in `grpc/common/responses/streaming.rs`.
// `generating` is the intermediate event for image_generation_call, on
// par with `searching` for web/file search and `interpreting` for code.
// `partial_image` events are emitted inline by the underlying tool when
// it streams preview chunks; the tool_loop path only emits the coarse
// in_progress → generating → completed sequence.
ResponseFormat::ImageGenerationCall => ImageGenerationCallEvent::GENERATING,
ResponseFormat::Passthrough => return true, // mcp_call has no intermediate event
};
Expand All @@ -491,7 +492,8 @@ fn send_tool_call_intermediate_event(
}

/// Send tool call completion events after tool execution.
/// Handles mcp_call, web_search_call, code_interpreter_call, and file_search_call items.
/// Handles mcp_call, web_search_call, code_interpreter_call, file_search_call,
/// and image_generation_call items.
/// Returns false if client disconnected.
fn send_tool_call_completion_events(
tx: &mpsc::UnboundedSender<Result<Bytes, io::Error>>,
Expand Down Expand Up @@ -559,8 +561,9 @@ fn stable_streaming_tool_item_id(
ResponseFormat::WebSearchCall => normalize_tool_item_id_with_prefix(source_id, "ws_"),
ResponseFormat::CodeInterpreterCall => normalize_tool_item_id_with_prefix(source_id, "ci_"),
ResponseFormat::FileSearchCall => normalize_tool_item_id_with_prefix(source_id, "fs_"),
// stubbed: `ig_` prefix matches the shared transformer's output item id
// (`to_image_generation_call`). R6.2 wires the full per-router path.
// `ig_` prefix mirrors the shared transformer's output item id
// (`to_image_generation_call`) and the 2-letter convention used by
// the other hosted tool formats.
ResponseFormat::ImageGenerationCall => normalize_tool_item_id_with_prefix(source_id, "ig_"),
}
}
Expand All @@ -583,8 +586,6 @@ fn non_streaming_tool_item_id_source(item_id: &str, response_format: &ResponseFo
ResponseFormat::WebSearchCall
| ResponseFormat::CodeInterpreterCall
| ResponseFormat::FileSearchCall
// stubbed: image_generation_call shares the fc_/call_ strip behavior
// with the other built-ins. R6.2 wires per-router specifics.
| ResponseFormat::ImageGenerationCall => item_id
.strip_prefix("fc_")
.or_else(|| item_id.strip_prefix("call_"))
Expand Down Expand Up @@ -1139,7 +1140,8 @@ fn build_mcp_approval_request_item(
/// Build a transformed output item using ResponseTransformer
///
/// Converts the output using the tool's response_format to the correctly-typed
/// output item (mcp_call, web_search_call, code_interpreter_call, file_search_call).
/// output item (mcp_call, web_search_call, code_interpreter_call, file_search_call,
/// image_generation_call).
/// Returns the result as a JSON Value for SSE event streaming.
fn build_transformed_mcp_call_item(
output: &Value,
Expand Down
10 changes: 8 additions & 2 deletions model_gateway/src/routers/openai/responses/streaming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ use futures_util::StreamExt;
use openai_protocol::{
event_types::{
is_function_call_type, is_response_event, CodeInterpreterCallEvent, FileSearchCallEvent,
FunctionCallEvent, ItemType, McpEvent, OutputItemEvent, ResponseEvent, WebSearchCallEvent,
FunctionCallEvent, ImageGenerationCallEvent, ItemType, McpEvent, OutputItemEvent,
ResponseEvent, WebSearchCallEvent,
},
responses::{ResponseTool, ResponsesRequest},
};
Expand Down Expand Up @@ -149,6 +150,9 @@ pub(super) fn apply_event_transformations_inplace(
// Determine item type and ID prefix based on response_format
let (new_type, id_prefix) = match response_format {
ResponseFormat::WebSearchCall => (ItemType::WEB_SEARCH_CALL, "ws_"),
ResponseFormat::ImageGenerationCall => {
(ItemType::IMAGE_GENERATION_CALL, "ig_")
}
Comment on lines +153 to +155

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The apply_event_transformations_inplace function is missing transformation logic for the CodeInterpreterCall and FileSearchCall response formats. While WebSearchCall and the newly added ImageGenerationCall are handled, these other built-in tool formats will currently fall through to the generic MCP_CALL type with an mcp_ ID prefix. To ensure consistency across all hosted tool formats as intended by this PR, they should be explicitly mapped to their respective item types and ID prefixes (ci_ and fs_). This ensures that the transformation logic correctly identifies the item variants present in the final response output.

                                ResponseFormat::CodeInterpreterCall => {
                                    (ItemType::CODE_INTERPRETER_CALL, "ci_")
                                }
                                ResponseFormat::FileSearchCall => (ItemType::FILE_SEARCH_CALL, "fs_"),
                                ResponseFormat::ImageGenerationCall => {
                                    (ItemType::IMAGE_GENERATION_CALL, "ig_")
                                }
References
  1. When processing response items, ensure the logic targets the specific item variants that are actually present in the final response output.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the gap — you're right that CodeInterpreterCall and FileSearchCall currently fall through to (ItemType::MCP_CALL, "mcp_") in this match. That predates R6.2; the existing WebSearchCall arm is the only explicit hosted-tool branch today. R6.1 / R6.2 are scoped to adding the image_generation_call path to match web_search_call, not to retrofit the other two formats.

Extending this match to also cover CodeInterpreterCall and FileSearchCall is a straightforward cleanup but touches behavior for two unrelated tool types, so it belongs in a follow-up rather than in the R6.x series (which is staged around the image_generation tool specifically). I'd rather keep this PR's blast radius narrow and file a separate PR that handles all the remaining hosted-tool formats uniformly.

Comment on lines +153 to +155

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Align image-generation argument event IDs

Rewriting ResponseFormat::ImageGenerationCall to image_generation_call with an ig_ item ID here causes a protocol mismatch because argument events in this same module are still emitted as response.mcp_call_arguments.* with mcp_* IDs (FunctionCallEvent::ARGUMENTS_DONE and send_buffered_arguments). For image-generation tool calls, clients now receive one call under two different item_id namespaces (ig_* vs mcp_*), which breaks correlation logic that joins argument deltas/done to the corresponding output item.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the P1 flag. To unpack it: the response.mcp_call_arguments.* events in this module (via FunctionCallEvent::ARGUMENTS_DONE and send_buffered_arguments) always rewrite item_id via mcp_response_item_id and stamp the type as McpEvent::CALL_ARGUMENTS_DONE regardless of response_format. That behavior predates R6.2 and applies uniformly to all three existing hosted-tool formats (web_search_call with ws_*, code_interpreter_call with ci_*, file_search_call with fs_*) — each of them already surfaces the same split-namespace pattern between the output_item's <prefix>_* id and the mcp_* argument-event item_id.

The R6.2 task is scoped to wiring image_generation_call through the same path as the other three hosted formats (the task instructions call out "stay consistent with the other 3 tools' shape; don't invent new patterns"). Fixing the argument-event namespace mismatch consistently for all four hosted formats — either by per-format id preservation or (more likely correct per OpenAI's spec) by suppressing mcp_call_arguments.* altogether for non-Passthrough formats — is a cross-cutting cleanup that belongs in a follow-up PR on top of R6.2/R6.3/R6.4 rather than a localized image-gen-only change that would itself diverge from the existing three-format shape.

Happy to pick that up as a follow-up once the R6.x series lands; leaving this PR narrowly scoped to image_generation wiring.

Comment on lines +153 to +155

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep image_generation item IDs consistent across tool events

Fresh evidence in this commit: apply_event_transformations_inplace now rewrites image-generation response.output_item.* entries to image_generation_call with an ig_ prefix, but this module still emits argument events as response.mcp_call_arguments.* and normalizes item_id with mcp_response_item_id(...) (mcp_*). For image-generation tool calls, clients that correlate argument and lifecycle events by item_id will now see two namespaces for the same call (ig_* vs mcp_*), which breaks joining argument delta/done to the corresponding added/in_progress/completed events.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a re-post of the same P1 from your previous review. See my reply on the original thread: #1356 (comment) — the response.mcp_call_arguments.* + mcp_response_item_id behavior is pre-existing across all three other hosted-tool formats (web_search_call, code_interpreter_call, file_search_call), not a regression introduced here. Fixing it consistently across all four formats is a separate cross-cutting cleanup tracked as a follow-up to the R6.x series.

_ => (ItemType::MCP_CALL, "mcp_"),
};

Expand Down Expand Up @@ -412,7 +416,8 @@ pub(super) fn forward_streaming_event(
}

/// Inject in_progress event after a tool call item is added.
/// Handles mcp_call, web_search_call, code_interpreter_call, and file_search_call items.
/// Handles mcp_call, web_search_call, code_interpreter_call, file_search_call,
/// and image_generation_call items.
/// Returns false if client disconnected.
fn maybe_inject_tool_in_progress(
parsed_data: &Value,
Expand All @@ -431,6 +436,7 @@ fn maybe_inject_tool_in_progress(
ItemType::WEB_SEARCH_CALL => WebSearchCallEvent::IN_PROGRESS,
ItemType::CODE_INTERPRETER_CALL => CodeInterpreterCallEvent::IN_PROGRESS,
ItemType::FILE_SEARCH_CALL => FileSearchCallEvent::IN_PROGRESS,
ItemType::IMAGE_GENERATION_CALL => ImageGenerationCallEvent::IN_PROGRESS,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: The docstring for this function (line 419) enumerates the handled item types but doesn't include image_generation_call after this addition. Consider updating it to stay in sync:

/// Handles mcp_call, web_search_call, code_interpreter_call, file_search_call, and image_generation_call items.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in c0dfc00 — updated the docstring to include image_generation_call in the enumerated handled types, plus two parallel docstrings in tool_loop.rs (send_tool_call_completion_events and build_transformed_mcp_call_item) that had the same stale enumeration.

_ => return true, // Not a tool call item, nothing to inject
};

Expand Down
Loading