-
Notifications
You must be signed in to change notification settings - Fork 7.2k
feat: return an error if the image sent by the user is a bad image #9146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| let event = EventMsg::Error(ErrorEvent { | ||
| message: "Invalid image in your last message. Please remove it and try again." | ||
| .to_string(), | ||
| codex_error_info: Some(CodexErrorInfo::BadRequest), | ||
| }); | ||
| sess.send_event(&turn_context, event).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for app server, we are still sending a generic EventMsg::Error with a specific message and I think we can parse that out.
Not sure if we are at a point for us to develop specific error types at this point just for app sever consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can create a new error type at https://github.com/openai/codex/blob/5029060ae7caba88abedc33a62fae8298fde3b66/codex-rs/protocol/src/protocol.rs and use it here to better document that this is a invalidUserInput and GUI and render as expected.
cc @owenlin0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure either but ping me if you change your mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add an InvalidUserImageError to CodexErrorInfo and here we can do codex_error_info: Some(CodexErrorInfo::InvalidUserImageError).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shijie-oai If we want to match on this error specifically and render something on the client, adding a more specific error type is a good idea
For tool call output we still replace it with a placeholder to avoid breaking the agentic loop bu tif this is because of a user message, we send an error to the user
Nice, I think this error should show up on turn/completed with turn.status failed, up to you @shijie-oai on what's nicer to use between that or the error notification on the client.
cc @celia-oai who implemented codex errors in v2 api
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a339b5c46e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ResponseItem::Message { role, .. } if role == "user" => false, | ||
| _ => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove invalid user images from history on error
When an InvalidImageRequest is triggered by a user-supplied image, this now returns false for user messages and leaves the invalid image in the conversation history. Because the history is reused for subsequent turns, a user who retries with a corrected message (without explicitly undoing/rolling back) will keep hitting the same invalid-image error, making the “remove it and try again” guidance ineffective. Consider removing or rolling back the offending user message when surfacing the error so retries can succeed.
Useful? React with 👍 / 👎.
Before
When we detect an
InvalidImageRequest, we replace the image by a placeholder and keep goingNow
In such
InvalidImageRequest, we check if the image is due to a user message or a tool call output. For tool call output we still replace it with a placeholder to avoid breaking the agentic loop bu tif this is because of a user message, we send an error to the user