diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 8986a2ecd64d..ca97280e1eaf 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -4465,27 +4465,28 @@ def run_conversation( or interim_has_codex_message_items ): last_msg = messages[-1] if messages else None - # Duplicate detection: two consecutive incomplete assistant - # messages with identical content AND reasoning are collapsed. - # For provider-state-only changes (encrypted reasoning - # items or replayable message ids/phases/statuses differ - # while visible content/reasoning are unchanged), compare - # those opaque payloads too so we don't silently drop the - # newer continuation state. - last_codex_items = last_msg.get("codex_reasoning_items") if isinstance(last_msg, dict) else None - interim_codex_items = interim_msg.get("codex_reasoning_items") - last_codex_message_items = last_msg.get("codex_message_items") if isinstance(last_msg, dict) else None - interim_codex_message_items = interim_msg.get("codex_message_items") - duplicate_interim = ( + # Duplicate detection: compare only visible content + # (content + reasoning). Opaque provider state + # (encrypted reasoning items, message item ids/phases) + # drifts per continuation even when the visible output + # is identical, so including it in the comparison defeats + # dedup and causes message storms (#52711). + visible_duplicate = ( isinstance(last_msg, dict) and last_msg.get("role") == "assistant" and last_msg.get("finish_reason") == "incomplete" and (last_msg.get("content") or "") == (interim_msg.get("content") or "") and (last_msg.get("reasoning") or "") == (interim_msg.get("reasoning") or "") - and last_codex_items == interim_codex_items - and last_codex_message_items == interim_codex_message_items ) - if not duplicate_interim: + if visible_duplicate: + # Update opaque state in-place so the latest + # provider payload is preserved without emitting + # a duplicate visible message. + for _key in ("codex_reasoning_items", "codex_message_items"): + _new_val = interim_msg.get(_key) + if _new_val is not None: + last_msg[_key] = _new_val + else: messages.append(interim_msg) agent._emit_interim_assistant_message(interim_msg) diff --git a/tests/run_agent/test_run_agent_codex_responses.py b/tests/run_agent/test_run_agent_codex_responses.py index 93823144a19b..f307b0b44579 100644 --- a/tests/run_agent/test_run_agent_codex_responses.py +++ b/tests/run_agent/test_run_agent_codex_responses.py @@ -1704,6 +1704,90 @@ def test_mid_turn_compaction_does_not_double_persist_in_place_rows(monkeypatch, ) +def _codex_incomplete_with_reasoning(text: str, reasoning_id: str = "rs_default"): + """Incomplete response with a reasoning item whose id/encrypted_content + can vary independently of the visible message text.""" + return SimpleNamespace( + output=[ + SimpleNamespace( + type="reasoning", + id=reasoning_id, + encrypted_content=f"opaque_{reasoning_id}", + summary=[SimpleNamespace(text="thinking...")], + ), + SimpleNamespace( + type="message", + status="in_progress", + content=[SimpleNamespace(type="output_text", text=text)], + ), + ], + usage=SimpleNamespace(input_tokens=4, output_tokens=2, total_tokens=6), + status="in_progress", + model="gpt-5-codex", + ) + + +def test_codex_incomplete_visible_dedup_suppresses_duplicate_interims(monkeypatch): + """Two consecutive incomplete responses with identical visible content + but different opaque reasoning items should be collapsed — only the first + interim is emitted to the user (#52711).""" + agent = _build_agent(monkeypatch) + # 2 incompletes with same text but different reasoning ids, then a final. + # (Only 2 to avoid hitting the cap of 3.) + responses = [ + _codex_incomplete_with_reasoning("Working on it...", "rs_1"), + _codex_incomplete_with_reasoning("Working on it...", "rs_2"), + _codex_message_response("Done."), + ] + monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0)) + + emitted: list = [] + original_emit = agent._emit_interim_assistant_message + def _capture_emit(msg): + emitted.append(msg.get("content")) + original_emit(msg) + monkeypatch.setattr(agent, "_emit_interim_assistant_message", _capture_emit) + + result = agent.run_conversation("test dedup") + + assert result["completed"] is True + # Only ONE interim should have been emitted (the first), not two. + assert len(emitted) == 1 + assert emitted[0] == "Working on it..." + + +def test_codex_incomplete_opaque_state_updated_in_place(monkeypatch): + """When visible content is a duplicate, the last message's opaque state + (codex_reasoning_items) should be updated in-place without emitting a new + interim (#52711).""" + agent = _build_agent(monkeypatch) + responses = [ + _codex_incomplete_with_reasoning("Partial output...", "rs_1"), + _codex_incomplete_with_reasoning("Partial output...", "rs_2"), + _codex_message_response("Final."), + ] + monkeypatch.setattr(agent, "_interruptible_api_call", lambda api_kwargs: responses.pop(0)) + + result = agent.run_conversation("test opaque update") + + assert result["completed"] is True + # Find the incomplete interim message in the result. + incompletes = [ + m for m in result["messages"] + if m.get("role") == "assistant" and m.get("finish_reason") == "incomplete" + ] + # Only one incomplete message should exist (the second was deduped). + assert len(incompletes) == 1 + # The opaque state should reflect the LATEST reasoning item (rs_2), + # updated in-place on the single message. + items = incompletes[0].get("codex_reasoning_items") + if items: + assert any( + (i.get("id") if isinstance(i, dict) else getattr(i, "id", None)) == "rs_2" + for i in items + ) + + def test_normalize_codex_response_marks_commentary_only_message_as_incomplete(monkeypatch): agent = _build_agent(monkeypatch) from agent.codex_responses_adapter import _normalize_codex_response @@ -2608,7 +2692,8 @@ def test_codex_message_item_status_survives_conversion_and_preflight(monkeypatch def test_duplicate_detection_distinguishes_different_codex_reasoning(monkeypatch): """Two consecutive reasoning-only responses with different encrypted content - must NOT be treated as duplicates.""" + are deduped on visible content — only one interim is kept, but opaque state + is updated in-place (#52711).""" agent = _build_agent(monkeypatch) responses = [ # First reasoning-only response @@ -2641,23 +2726,23 @@ def test_duplicate_detection_distinguishes_different_codex_reasoning(monkeypatch assert result["completed"] is True assert result["final_response"] == "Final answer after thinking." - # Both reasoning-only interim messages should be in history (not collapsed) + # Only one reasoning-only interim should be in history (deduped on + # visible content — both have empty visible output). interim_msgs = [ msg for msg in result["messages"] if msg.get("role") == "assistant" and msg.get("finish_reason") == "incomplete" ] - assert len(interim_msgs) == 2 - encrypted_contents = [ - msg["codex_reasoning_items"][0]["encrypted_content"] - for msg in interim_msgs - ] - assert "enc_first" in encrypted_contents - assert "enc_second" in encrypted_contents + assert len(interim_msgs) == 1 + # But the opaque state should reflect the LATEST reasoning item. + items = interim_msgs[0].get("codex_reasoning_items") + if items: + assert items[0].get("encrypted_content") == "enc_second" def test_duplicate_detection_distinguishes_different_codex_message_items(monkeypatch): - """Incomplete turns with new message ids/phases/statuses must not be collapsed.""" + """Incomplete turns with same visible content but different message ids + are deduped — only one interim kept, opaque state updated in-place (#52711).""" agent = _build_agent(monkeypatch) responses = [ SimpleNamespace( @@ -2700,12 +2785,12 @@ def test_duplicate_detection_distinguishes_different_codex_message_items(monkeyp if msg.get("role") == "assistant" and msg.get("finish_reason") == "incomplete" ] - assert len(interim_msgs) == 2 - assert [msg["codex_message_items"][0]["id"] for msg in interim_msgs] == [ - "msg_first", - "msg_second", - ] - assert all(msg["codex_message_items"][0]["status"] == "in_progress" for msg in interim_msgs) + # Only one interim — deduped on visible content ("Still working..." == "Still working..."). + assert len(interim_msgs) == 1 + # Opaque state should reflect the latest message item. + items = interim_msgs[0].get("codex_message_items") + if items: + assert items[0].get("id") == "msg_second" def test_chat_messages_to_responses_input_deduplicates_reasoning_ids(monkeypatch):