Revert "feat(image): add image generation tool support" - #1087
Conversation
This reverts commit 9bd9645.
|
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 (20)
📝 WalkthroughWalkthroughThis PR removes comprehensive image-generation tool support from the codebase, including enum variants, type definitions, transformer functions, routing logic, event handling, re-exports, and utility functions across MCP, protocols, and model gateway components. JSON value parameter types are standardized to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request removes image generation tool support across the MCP and protocols crates, deleting associated enum variants, logic, and the tool_output_context utility. Feedback identifies a regression where converting tool outputs to strings via .to_string() introduces unnecessary quotes into conversation histories, which should be corrected to maintain raw text. Additionally, it is recommended to replace the newly introduced boolean success flag with an HTTP status code to properly support circuit breaker functionality when recording tool outcomes.
|
|
||
| let model_context_output = | ||
| compact_tool_output_for_model_context(&response_format, &tool_output.output); | ||
| let output_str = tool_output.output.to_string(); |
There was a problem hiding this comment.
Using to_string() on a serde_json::Value that is a string will include the surrounding double quotes in the resulting String. This is a regression from the previous logic (which used compact_tool_output_for_model_context) and can lead to malformed conversation history being sent back to the model, as tool outputs are expected to be raw text when possible.
| let output_str = tool_output.output.to_string(); | |
| let output_str = tool_output.output.as_str().map(String::from).unwrap_or_else(|| tool_output.output.to_string()); |
| }, | ||
| ); | ||
|
|
||
| let output_str = tool_output.output.to_string(); |
There was a problem hiding this comment.
Similar to the streaming path, using to_string() directly on the tool output value will introduce unnecessary quotes if the output is a plain string, which degrades the quality of the conversation history sent to the model.
| let output_str = tool_output.output.to_string(); | |
| let output_str = tool_output.output.as_str().map(String::from).unwrap_or_else(|| tool_output.output.to_string()); |
| output_str: String, | ||
| response_format: ResponseFormat, | ||
| output_item: ResponseOutputItem, | ||
| _success: bool, |
There was a problem hiding this comment.
The _success parameter (boolean) should be replaced with an HTTP status code. According to repository rules, when recording the outcome of a worker request, the record_outcome function must be called with the HTTP status code rather than a boolean. This allows the worker's internal logic to determine if the status code represents a circuit breaker failure.
References
- When recording the outcome of a worker request, ensure that the record_outcome function is called with the HTTP status code, not a boolean indicating success or failure. The worker's internal logic will determine if the status code represents a circuit breaker failure based on its configured retryable status codes.
Reverts #1057
This PR only included e2e tests for gpt-5.1 models. However, it touched couple of files in grpc routers.
The changes here are not fully covered in e2e tests.
Summary by CodeRabbit