Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/onyx/tools/tool_runner.py">
<violation number="1" location="backend/onyx/tools/tool_runner.py:159">
P2: Avoid logging the raw exception string; it may contain sensitive URLs/tokens. Log only safe metadata like the exception type or status code instead.
(Based on your team's feedback about not logging raw exception strings that may contain URLs with temporary auth tokens.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR introduces error propagation for image generation failures but contains critical bugs that prevent it from working. Major Issues:
What needs to be fixed:
Impact: Without these fixes, users will see a stuck "Generating image..." spinner when image generation fails, defeating the purpose of this PR. Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant API as Chat API
participant ToolRunner
participant ImageTool as Image Generation Tool
participant OpenAI as OpenAI API
participant Emitter
User->>Frontend: Request image generation
Frontend->>API: POST /send-chat-message
API->>ToolRunner: run_tool_calls()
ToolRunner->>ImageTool: run(placement, **kwargs)
ImageTool->>Emitter: emit(ImageGenerationToolStart)
Emitter->>Frontend: Stream packet (image_generation_start)
ImageTool->>OpenAI: generate_image()
alt Success
OpenAI-->>ImageTool: Image data
ImageTool->>Emitter: emit(ImageGenerationFinal)
Emitter->>Frontend: Stream packet (image_generation_final)
ImageTool->>Emitter: emit(SectionEnd)
Emitter->>Frontend: Stream packet (section_end)
else Error (Content Policy, Rate Limit, etc)
OpenAI-->>ImageTool: Exception
ImageTool->>ImageTool: raise ToolExecutionException(message, emit_error_packet=True)
Note over ImageTool: ❌ BUG: Exception bubbles up
ImageTool-->>ToolRunner: ToolExecutionException
ToolRunner->>ToolRunner: catch ToolExecutionException
ToolRunner->>Emitter: emit(PacketException(exception=e))
Note over Emitter: ❌ BUG: Exception object, not string
Emitter->>Frontend: Stream packet with Exception object
Note over Frontend: ❌ BUG: Expects {type: "error", message: string}
Frontend->>Frontend: ImageToolRenderer error=false (hardcoded)
Frontend->>User: Shows "Generating image..." (stuck)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Comments (3)
-
backend/onyx/server/query_and_chat/streaming_models.py, line 74-78 (link)logic: The
exceptionfield should be a string to properly serialize for the frontend. The frontend expectsPacketErrorwithmessage?: string, but this stores an Exception object. -
web/src/app/chat/message/messageComponents/renderers/ImageToolRenderer.tsx, line 40-41 (link)logic: The error state is hardcoded to
falseand never extracted from packets. Need to check forPacketType.ERRORin the packets and extract the error message. -
web/src/app/chat/message/messageComponents/renderers/ImageToolRenderer.tsx, line 153-163 (link)style: The error display should show the actual error message from the backend, not just "Image generation failed".
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
3 files reviewed, 4 comments
| if e.emit_error_packet: | ||
| tool.emitter.emit( | ||
| Packet( | ||
| placement=tool_call.placement, | ||
| obj=PacketException(exception=e), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
logic: Critical bug: PacketException stores the exception object directly, but frontend expects a message string field. When model_dump() serializes this for SSE streaming (chat_backend.py:521), the Exception object won't serialize to the format the frontend expects.
The frontend interface is:
export interface PacketError extends BaseObj {
type: "error";
message?: string;
}But the backend PacketException has:
exception: Exception # Not a string!| if e.emit_error_packet: | |
| tool.emitter.emit( | |
| Packet( | |
| placement=tool_call.placement, | |
| obj=PacketException(exception=e), | |
| ) | |
| ) | |
| if e.emit_error_packet: | |
| tool.emitter.emit( | |
| Packet( | |
| placement=tool_call.placement, | |
| obj=PacketException(exception=str(e)), | |
| ) | |
| ) |
Note: Also need to update PacketException model to accept str instead of Exception type, or add a @field_serializer to convert exception to string.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/onyx/tools/tool_runner.py
Line: 177:183
Comment:
**logic:** Critical bug: `PacketException` stores the exception object directly, but frontend expects a `message` string field. When `model_dump()` serializes this for SSE streaming (chat_backend.py:521), the Exception object won't serialize to the format the frontend expects.
The frontend interface is:
```typescript
export interface PacketError extends BaseObj {
type: "error";
message?: string;
}
```
But the backend `PacketException` has:
```python
exception: Exception # Not a string!
```
```suggestion
if e.emit_error_packet:
tool.emitter.emit(
Packet(
placement=tool_call.placement,
obj=PacketException(exception=str(e)),
)
)
```
Note: Also need to update `PacketException` model to accept `str` instead of `Exception` type, or add a `@field_serializer` to convert exception to string.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
This seems incorrect. PacketException accepts an exception which is of type Exception
There was a problem hiding this comment.
Comment is incorrect
This seems incorrect. Packet Exception accepts an Exception in it, not Str. The exception is then raised in run_chat_loop_with_state_containers and then caught in handle_stream_message_objects, where a StreamingError is yielded to the client. |
2 participants
Description
When the image gen tool fails, the user has no insight as to why. There are a multitude of reasons as to why this could fail (eg. moderation, rate limiting, bad request, ...).
This PR enables us to propagate the image generation errors up to the user
Example Look:
How Has This Been Tested?
Manual
Additional Options
Future extensions
Creating an image-gen error packet for custom display
Update other tools to make use of this new exception instead of raising ValueErrors
Custom frontend display and nicer handling of these errors
[Optional] Override Linear Check
Summary by cubic
Surface image generation errors to the user by emitting explicit error packets and clearer messages. Improves visibility for failures like moderation blocks, invalid input, and fetch/convert errors.
Written for commit 978109c. Summary will update on new commits.