Conversation
|
@codex review |
![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: 0257728856
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51062332c2
ℹ️ 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".
| #[serde(untagged)] | ||
| pub enum TurnOutcome { | ||
| Succeeded { last_agent_message: Option<String> }, | ||
| Failed { error: ErrorEvent }, |
There was a problem hiding this comment.
TurnOutcome is #[serde(untagged)] with Succeeded { last_agent_message: Option<String> } listed before Failed { error: ErrorEvent }. Because unknown fields are ignored, payloads like { "error": ... } can deserialize as Succeeded { last_agent_message: None }, dropping failure details. This can misclassify failed turns after JSON round-trips (e.g., rollout replay/resume).
Useful? React with 👍 / 👎.
| codex_protocol::protocol::TurnOutcome::Failed { error } => { | ||
| self.on_error(error.message); |
There was a problem hiding this comment.
The failed TurnComplete branch calls on_error(error.message) directly, which bypasses the existing EventMsg::Error logic that interprets codex_error_info (e.g., server overload and steer-rejection handling). As a result, failed turns with structured error metadata are treated as generic errors and lose specialized UX/recovery behavior.
Useful? React with 👍 / 👎.
| TurnOutcome::Failed { error } => { | ||
| turn.status = TurnStatus::Failed; | ||
| turn.error = Some(V2TurnError { | ||
| message: error.message.clone(), |
There was a problem hiding this comment.
Thread-history reconstruction marks every TurnOutcome::Failed as TurnStatus::Failed unconditionally. Live app-server handling now gates failure on error.affects_turn_status(), so replay/resume can diverge from live behavior and incorrectly mark non-fatal failures as failed turns.
Useful? React with 👍 / 👎.
1 participant
No description provided.