mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
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
This commit is contained in:
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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<ResponseItem> {
|
||||
match input {
|
||||
ResponseInputItem::FunctionCallOutput { call_id, output } => {
|
||||
|
||||
@@ -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<MessagePhase>,
|
||||
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")]
|
||||
|
||||
@@ -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",
|
||||
}
|
||||
})),
|
||||
|
||||
@@ -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.
|
||||
(
|
||||
|
||||
Reference in New Issue
Block a user