From 2aa84b889140903ec809a99362ca49dfcc46cb7d Mon Sep 17 00:00:00 2001 From: dedrisian-oai Date: Sun, 14 Sep 2025 17:34:33 -0700 Subject: [PATCH] Fix EventMsg Optional (#3604) --- codex-rs/core/src/codex.rs | 5 ++-- codex-rs/core/tests/suite/review.rs | 40 ++++++++++++++++++++++++----- codex-rs/protocol/src/protocol.rs | 8 +++++- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7177348f..375183ab 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -18,6 +18,7 @@ use codex_apply_patch::MaybeApplyPatchVerified; use codex_apply_patch::maybe_parse_apply_patch_verified; use codex_protocol::mcp_protocol::ConversationId; use codex_protocol::protocol::ConversationPathResponseEvent; +use codex_protocol::protocol::ExitedReviewModeEvent; use codex_protocol::protocol::ReviewRequest; use codex_protocol::protocol::RolloutItem; use codex_protocol::protocol::TaskStartedEvent; @@ -3249,11 +3250,11 @@ fn convert_call_tool_result_to_function_call_output_payload( async fn exit_review_mode( session: Arc, task_sub_id: String, - res: Option, + review_output: Option, ) { let event = Event { id: task_sub_id, - msg: EventMsg::ExitedReviewMode(res), + msg: EventMsg::ExitedReviewMode(ExitedReviewModeEvent { review_output }), }; session.send_event(event).await; } diff --git a/codex-rs/core/tests/suite/review.rs b/codex-rs/core/tests/suite/review.rs index 2bcb67ff..f65913a2 100644 --- a/codex-rs/core/tests/suite/review.rs +++ b/codex-rs/core/tests/suite/review.rs @@ -5,6 +5,7 @@ use codex_core::ModelProviderInfo; use codex_core::built_in_model_providers; use codex_core::config::Config; use codex_core::protocol::EventMsg; +use codex_core::protocol::ExitedReviewModeEvent; use codex_core::protocol::InputItem; use codex_core::protocol::Op; use codex_core::protocol::ReviewCodeLocation; @@ -89,8 +90,10 @@ async fn review_op_emits_lifecycle_and_review_output() { let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; let review = match closed { - EventMsg::ExitedReviewMode(Some(r)) => r, - other => panic!("expected ExitedReviewMode(Some(..)), got {other:?}"), + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), }; // Deep compare full structure using PartialEq (floats are f32 on both sides). @@ -153,8 +156,10 @@ async fn review_op_with_plain_text_emits_review_fallback() { let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; let closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(_))).await; let review = match closed { - EventMsg::ExitedReviewMode(Some(r)) => r, - other => panic!("expected ExitedReviewMode(Some(..)), got {other:?}"), + EventMsg::ExitedReviewMode(ev) => ev + .review_output + .expect("expected ExitedReviewMode with Some(review_output)"), + other => panic!("expected ExitedReviewMode(..), got {other:?}"), }; // Expect a structured fallback carrying the plain text. @@ -283,7 +288,15 @@ async fn review_uses_custom_review_model_from_config() { // Wait for completion let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; - let _closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(None))).await; + let _closed = wait_for_event(&codex, |ev| { + matches!( + ev, + EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: None + }) + ) + }) + .await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; // Assert the request body model equals the configured review model @@ -395,7 +408,15 @@ async fn review_input_isolated_from_parent_history() { .unwrap(); let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; - let _closed = wait_for_event(&codex, |ev| matches!(ev, EventMsg::ExitedReviewMode(None))).await; + let _closed = wait_for_event(&codex, |ev| { + matches!( + ev, + EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: None + }) + ) + }) + .await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; // Assert the request `input` contains only the single review user message. @@ -449,7 +470,12 @@ async fn review_history_does_not_leak_into_parent_session() { .unwrap(); let _entered = wait_for_event(&codex, |ev| matches!(ev, EventMsg::EnteredReviewMode(_))).await; let _closed = wait_for_event(&codex, |ev| { - matches!(ev, EventMsg::ExitedReviewMode(Some(_))) + matches!( + ev, + EventMsg::ExitedReviewMode(ExitedReviewModeEvent { + review_output: Some(_) + }) + ) }) .await; let _complete = wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await; diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 6405bf0c..c3aebcdd 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -414,6 +414,7 @@ pub struct Event { } /// Response event from the agent +/// NOTE: Make sure none of these values have optional types, as it will mess up the extension code-gen. #[derive(Debug, Clone, Deserialize, Serialize, Display, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] @@ -514,7 +515,12 @@ pub enum EventMsg { EnteredReviewMode(ReviewRequest), /// Exited review mode with an optional final result to apply. - ExitedReviewMode(Option), + ExitedReviewMode(ExitedReviewModeEvent), +} + +#[derive(Debug, Clone, Deserialize, Serialize, TS)] +pub struct ExitedReviewModeEvent { + pub review_output: Option, } // Individual event payload types matching each `EventMsg` variant.