From b337fe06b7d8d6e5b42295e04c5e92c2b9c46567 Mon Sep 17 00:00:00 2001 From: eknight-oai Date: Thu, 22 Oct 2026 09:57:25 +0000 Subject: [PATCH] Defer agent mail after empty final answers (#56133) ## Why An explicit `final_answer` with empty or whitespace-only text could allow queued agent mail to restart the turn instead of leaving it queued for the next turn. ## What changed Treat explicit `final_answer` messages as terminal regardless of visible text in both completed-item handling and turn-item finalization. Untagged messages still require visible text to defer mailbox delivery; `commentary` and `partial_answer` messages do not defer it. ## Testing Extend regression coverage for empty and whitespace-only final answers, delivery of queued agent mail on the next turn, and message board notifications arriving before or after an empty final answer. GitOrigin-RevId: 466f1a9c613e07d67ce45458858ba32d5d23f11d --- codex-rs/core/src/state/turn.rs | 7 ++--- codex-rs/core/src/stream_events_utils.rs | 28 +++++++++++++------ .../core/src/stream_events_utils_tests.rs | 19 +++++++++---- codex-rs/core/tests/suite/pending_input.rs | 9 ++++-- .../suite/scenarios_agent_message_board.rs | 13 ++++++--- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index aa43776d86..0fe0fea1bb 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -40,9 +40,8 @@ pub(crate) struct ActiveTurn { /// State machine: /// - A turn starts in `CurrentTurn`, so queued child mail can join the next /// model request for that turn. -/// - After user-visible terminal output is recorded, we switch to `NextTurn` -/// to leave late child mail queued instead of extending an already shown -/// answer. +/// - After a terminal answer is recorded, even an empty one, we switch to +/// `NextTurn` to leave late child mail queued instead of extending the answer. /// - If the same task later gets explicit same-turn work again (a steered user /// prompt or a tool call after an untagged preamble), we reopen `CurrentTurn` /// so that pending child mail is drained into that follow-up request. @@ -51,7 +50,7 @@ pub(crate) enum MailboxDeliveryPhase { /// Incoming mailbox messages can still be consumed by the current turn. #[default] CurrentTurn, - /// The current turn already emitted visible final answer text; mailbox + /// The current turn already emitted a terminal answer; mailbox /// messages should remain queued for a later turn. NextTurn, } diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index 1b668ea4bb..964302c4f1 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -292,8 +292,9 @@ pub(crate) async fn finalize_non_tool_response_item( Some(combined) }; let defers_mailbox_delivery_to_next_turn = - matches!(agent_message.phase, Some(MessagePhase::FinalAnswer) | None) - && last_agent_message.is_some(); + assistant_message_defers_mailbox_delivery(agent_message.phase.as_ref(), || { + last_agent_message.is_some() + }); ( agent_message.memory_citation.clone(), last_agent_message, @@ -523,18 +524,27 @@ fn completed_item_defers_mailbox_delivery_to_next_turn( plan_mode: bool, ) -> bool { match item { - ResponseItem::Message { role, phase, .. } => { - if role != "assistant" || !matches!(phase, Some(MessagePhase::FinalAnswer) | None) { - return false; - } - // Treat `None` like final-answer text so untagged providers default - // to the safer "defer mailbox mail" behavior. - last_assistant_message_from_item(item, plan_mode).is_some() + ResponseItem::Message { role, phase, .. } if role == "assistant" => { + assistant_message_defers_mailbox_delivery(phase.as_ref(), || { + last_assistant_message_from_item(item, plan_mode).is_some() + }) } _ => false, } } +fn assistant_message_defers_mailbox_delivery( + phase: Option<&MessagePhase>, + has_visible_text: impl FnOnce() -> bool, +) -> bool { + match phase { + Some(MessagePhase::FinalAnswer) => true, + Some(MessagePhase::Commentary | MessagePhase::PartialAnswer) => false, + // Without an explicit phase, keep treating visible assistant text as a final answer. + None => has_visible_text(), + } +} + pub(crate) fn response_input_to_response_item(input: &ResponseInputItem) -> Option { match input { ResponseInputItem::FunctionCallOutput { call_id, output } => { diff --git a/codex-rs/core/src/stream_events_utils_tests.rs b/codex-rs/core/src/stream_events_utils_tests.rs index b942ac7364..d402653316 100644 --- a/codex-rs/core/src/stream_events_utils_tests.rs +++ b/codex-rs/core/src/stream_events_utils_tests.rs @@ -549,13 +549,20 @@ fn last_assistant_message_from_item_returns_none_for_plan_only_hidden_message() ); } -#[test] -fn completed_item_defers_mailbox_delivery_for_unknown_phase_messages() { - let item = assistant_output_text("final answer"); +#[test_case::test_case(Some(MessagePhase::FinalAnswer), "", true; "explicit_empty_final")] +#[test_case::test_case(None, "final answer", true; "untagged_visible_answer")] +#[test_case::test_case(None, "", false; "untagged_empty_message")] +fn completed_item_defers_mailbox_delivery_for_final_answers( + phase: Option, + text: &str, + expected: bool, +) { + let item = assistant_output_text_with_phase(text, phase); - assert!(completed_item_defers_mailbox_delivery_to_next_turn( - &item, /*plan_mode*/ false, - )); + assert_eq!( + completed_item_defers_mailbox_delivery_to_next_turn(&item, /*plan_mode*/ false), + expected + ); } #[test_case::test_case(MessagePhase::Commentary; "commentary")] diff --git a/codex-rs/core/tests/suite/pending_input.rs b/codex-rs/core/tests/suite/pending_input.rs index 4a08e101b7..d7ce6b5af9 100644 --- a/codex-rs/core/tests/suite/pending_input.rs +++ b/codex-rs/core/tests/suite/pending_input.rs @@ -1152,19 +1152,22 @@ async fn queued_inter_agent_mail_triggers_follow_up_after_commentary_message_ite server.shutdown().await; } +#[test_case("first answer"; "visible_final")] +#[test_case(""; "empty_final")] +#[test_case(" \n\t"; "whitespace_final")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn queued_inter_agent_mail_does_not_restart_after_final_answer() { +async fn queued_inter_agent_mail_does_not_restart_after_final_answer(final_answer: &str) { let first_chunks = vec![ chunk(ev_response_created("resp-1")), chunk(ev_message_item_added("msg-1", "")), - chunk(ev_output_text_delta("first answer")), + chunk(ev_output_text_delta(final_answer)), chunk(json!({ "type": "response.output_item.done", "item": { "type": "message", "role": "assistant", "id": "msg-1", - "content": [{"type": "output_text", "text": "first answer"}], + "content": [{"type": "output_text", "text": final_answer}], "phase": "final_answer", } })), diff --git a/codex-rs/core/tests/suite/scenarios_agent_message_board.rs b/codex-rs/core/tests/suite/scenarios_agent_message_board.rs index c1bece7329..8df6c97235 100644 --- a/codex-rs/core/tests/suite/scenarios_agent_message_board.rs +++ b/codex-rs/core/tests/suite/scenarios_agent_message_board.rs @@ -388,10 +388,15 @@ async fn board_is_shared_with_children_and_skips_idle_notices( Ok(()) } -#[test_case::test_case(false; "before_final")] -#[test_case::test_case(true; "after_final")] +#[test_case::test_case(false, "Finished."; "before_final")] +#[test_case::test_case(true, "Finished."; "after_final")] +#[test_case::test_case(false, ""; "before_empty_final")] +#[test_case::test_case(true, ""; "after_empty_final")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn board_notifications_do_not_reopen_a_final_answer(after_final: bool) -> anyhow::Result<()> { +async fn board_notifications_do_not_reopen_a_final_answer( + after_final: bool, + final_answer: &str, +) -> anyhow::Result<()> { let response = |body| vec![StreamingSseChunk { gate: None, body }]; let (release_final, final_gate) = oneshot::channel(); let (release_child, child_gate) = oneshot::channel(); @@ -400,7 +405,7 @@ async fn board_notifications_do_not_reopen_a_final_answer(after_final: bool) -> event }; let started = final_phase(ev_message_item_added("root-final", "")); - let finished = final_phase(ev_assistant_message("root-final", "Finished.")); + let finished = final_phase(ev_assistant_message("root-final", final_answer)); let (before_gate, after_gate) = if after_final { // Starting an empty item confirms the answer was processed without adding mailbox work. (