Python: Stop emitting duplicate reasoning content from OpenAI response.reasoning_text.done and response.reasoning_summary_text.done events (#5162)

* Fix reasoning text done events duplicating streamed delta content (#5157) The OpenAI Responses API sends both reasoning_text.delta (incremental chunks) and reasoning_text.done (full accumulated text) events. The chat client was emitting Content for both, causing ag-ui to append the full done text onto already-accumulated delta text, producing duplicated reasoning output. Stop emitting Content for reasoning_text.done and reasoning_summary_text.done events, matching how output_text.done is already handled (not emitted). The deltas contain all the content; the done event is redundant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(openai): emit reasoning done content as fallback when no deltas observed (#5157) Address PR review feedback: - Track item_ids that received reasoning deltas via seen_reasoning_delta_item_ids set - Emit content from done events only when no deltas were received for the item_id, preventing silent content loss on stream resumption - Add comment documenting code_interpreter done event asymmetry - Replace redundant ag-ui test with deduplication-focused test - Add integration test for delta+done sequence in OpenAI chat client tests - Add fallback path tests for done events without preceding deltas Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for #5157: Python: [Bug]: "type": "response.reasoning_text.delta" and "response.reasoning_text.done" both get exposed as "text_reasoning" * Fix AG-UI reasoning streaming to use proper Start/End pattern (#5157) _emit_text_reasoning now follows the same streaming pattern as _emit_text: - Emits ReasoningStartEvent/ReasoningMessageStartEvent only on the first delta for a given message_id - Emits only ReasoningMessageContentEvent for subsequent deltas - Defers ReasoningMessageEndEvent/ReasoningEndEvent until _close_reasoning_block is called (on content type switch or end-of-run) This produces the correct protocol pattern: ReasoningStartEvent ReasoningMessageStartEvent ReasoningMessageContentEvent(delta1) ReasoningMessageContentEvent(delta2) ReasoningMessageEndEvent ReasoningEndEvent Instead of wrapping every delta in a full Start→End sequence. Backward compatibility is preserved: calling _emit_text_reasoning without a flow argument still produces the full sequence per call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix import ordering lint error in AG-UI test file (#5157) Move inline import of TextMessageContentEvent to the top-level import block and ensure alphabetical ordering to satisfy ruff I001 rule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix mypy error: rename loop variable to avoid type conflict with WorkflowEvent The 'event' variable was already typed as WorkflowEvent[Any] from the async for loop at line 590. Reusing it in the _close_reasoning_block loop (which returns list[BaseEvent]) caused an incompatible assignment error. Renamed to 'reasoning_evt' to avoid the conflict. Fixes #5162 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review feedback for #5157: review comment fixes * narrow test result reporting to explicit pytest JUnit XML * Fix test args * Fix pytest-results-action in merge workflow and remove committed test artifacts Apply the same JUnit XML fix from python-tests.yml to python-merge-tests.yml: add --junitxml=pytest.xml to all test commands and narrow the results action path from ./python/**.xml to ./python/pytest.xml. Also remove accidentally committed pytest.xml and python-coverage.xml and add them to .gitignore. --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-06-16 21:04:09 +08:00 · 2026-04-10 07:44:59 +09:00
parent 1dd828d255
commit 5e8fe0be1f
15 changed files with 412 additions and 89 deletions
@@ -11,6 +11,7 @@ from ag_ui.core import (
    ReasoningMessageEndEvent,
    ReasoningMessageStartEvent,
    ReasoningStartEvent,
+    TextMessageContentEvent,
    TextMessageEndEvent,
    TextMessageStartEvent,
    ToolCallArgsEvent,
@@ -29,6 +30,7 @@ from agent_framework_ag_ui._agent_run import (
 from agent_framework_ag_ui._run_common import (
    FlowState,
    _build_run_finished_event,
+    _close_reasoning_block,
    _emit_approval_request,
    _emit_content,
    _emit_mcp_tool_call,
@@ -1344,8 +1346,11 @@ class TestEmitContentMcpRouting:

        events = _emit_content(content, flow)

-        assert len(events) == 5
+        # Streaming pattern: Start + MessageStart + Content (no End events yet)
+        assert len(events) == 3
        assert isinstance(events[0], ReasoningStartEvent)
+        assert isinstance(events[1], ReasoningMessageStartEvent)
+        assert isinstance(events[2], ReasoningMessageContentEvent)


 class TestReasoningInSnapshot:
@@ -1501,3 +1506,137 @@ class TestReasoningInSnapshot:
        assert len(flow.reasoning_messages) == 1
        assert flow.reasoning_messages[0]["content"] == "part1 part2"
        assert flow.reasoning_messages[0]["encryptedValue"] == "encrypted-payload"
+
+    def test_reasoning_done_after_deltas_does_not_duplicate(self):
+        """A done-style content arriving after deltas does not duplicate accumulated text.
+
+        The upstream client should skip done events when deltas preceded them,
+        but if one leaks through, the accumulator must not double-append.
+        This test verifies that only the delta-produced text is stored.
+        """
+        flow = FlowState()
+        msg_id = "reason_dedup"
+
+        delta1 = Content.from_text_reasoning(id=msg_id, text="Hello ")
+        delta2 = Content.from_text_reasoning(id=msg_id, text="world")
+
+        _emit_text_reasoning(delta1, flow)
+        _emit_text_reasoning(delta2, flow)
+
+        # Accumulated text should equal the concatenation of deltas only
+        assert len(flow.reasoning_messages) == 1
+        assert flow.reasoning_messages[0]["content"] == "Hello world"
+        assert flow.reasoning_messages[0]["id"] == msg_id
+
+    def test_reasoning_deltas_emit_one_content_event_each(self):
+        """Each reasoning delta emits exactly one ReasoningMessageContentEvent
+        within a single Start/End sequence (streaming pattern)."""
+        flow = FlowState()
+        msg_id = "reason_evt"
+
+        delta1 = Content.from_text_reasoning(id=msg_id, text="Think ")
+        delta2 = Content.from_text_reasoning(id=msg_id, text="hard")
+
+        events1 = _emit_text_reasoning(delta1, flow)
+        events2 = _emit_text_reasoning(delta2, flow)
+        close_events = _close_reasoning_block(flow)
+
+        all_events = events1 + events2 + close_events
+        content_events = [e for e in all_events if isinstance(e, ReasoningMessageContentEvent)]
+
+        assert len(content_events) == 2
+        assert content_events[0].delta == "Think "
+        assert content_events[1].delta == "hard"
+
+        # Streaming pattern: one Start/End sequence wrapping both content events
+        start_events = [e for e in all_events if isinstance(e, ReasoningStartEvent)]
+        end_events = [e for e in all_events if isinstance(e, ReasoningEndEvent)]
+        msg_start_events = [e for e in all_events if isinstance(e, ReasoningMessageStartEvent)]
+        msg_end_events = [e for e in all_events if isinstance(e, ReasoningMessageEndEvent)]
+        assert len(start_events) == 1
+        assert len(end_events) == 1
+        assert len(msg_start_events) == 1
+        assert len(msg_end_events) == 1
+
+    def test_reasoning_streaming_event_order(self):
+        """Streaming reasoning emits Start once, then Content per delta, then End on close."""
+        flow = FlowState()
+        msg_id = "reason_order"
+
+        d1 = Content.from_text_reasoning(id=msg_id, text="A ")
+        d2 = Content.from_text_reasoning(id=msg_id, text="B ")
+        d3 = Content.from_text_reasoning(id=msg_id, text="C")
+
+        events = []
+        events.extend(_emit_text_reasoning(d1, flow))
+        events.extend(_emit_text_reasoning(d2, flow))
+        events.extend(_emit_text_reasoning(d3, flow))
+        events.extend(_close_reasoning_block(flow))
+
+        assert isinstance(events[0], ReasoningStartEvent)
+        assert isinstance(events[1], ReasoningMessageStartEvent)
+        assert isinstance(events[2], ReasoningMessageContentEvent)
+        assert events[2].delta == "A "
+        assert isinstance(events[3], ReasoningMessageContentEvent)
+        assert events[3].delta == "B "
+        assert isinstance(events[4], ReasoningMessageContentEvent)
+        assert events[4].delta == "C"
+        assert isinstance(events[5], ReasoningMessageEndEvent)
+        assert isinstance(events[6], ReasoningEndEvent)
+        assert len(events) == 7
+
+    def test_close_reasoning_block_noop_when_not_open(self):
+        """_close_reasoning_block returns empty list when no reasoning block is open."""
+        flow = FlowState()
+        assert _close_reasoning_block(flow) == []
+
+    def test_close_reasoning_block_resets_state(self):
+        """_close_reasoning_block clears reasoning_message_id."""
+        flow = FlowState()
+        _emit_text_reasoning(Content.from_text_reasoning(id="r1", text="x"), flow)
+        assert flow.reasoning_message_id == "r1"
+
+        _close_reasoning_block(flow)
+        assert flow.reasoning_message_id is None
+
+    def test_emit_content_closes_reasoning_on_text(self):
+        """Switching from reasoning to text content auto-closes reasoning block."""
+        flow = FlowState()
+        reasoning = Content.from_text_reasoning(id="r1", text="thinking")
+        text = Content.from_text("answer")
+
+        r_events = _emit_content(reasoning, flow)
+        t_events = _emit_content(text, flow)
+
+        # reasoning events: Start + MsgStart + Content
+        assert isinstance(r_events[0], ReasoningStartEvent)
+        # text events should start with reasoning End events
+        assert isinstance(t_events[0], ReasoningMessageEndEvent)
+        assert isinstance(t_events[1], ReasoningEndEvent)
+        # then text start
+
+        assert isinstance(t_events[2], TextMessageStartEvent)
+        assert isinstance(t_events[3], TextMessageContentEvent)
+
+    def test_reasoning_distinct_ids_close_previous_block(self):
+        """Emitting reasoning with a new message_id auto-closes the previous block."""
+        flow = FlowState()
+        c1 = Content.from_text_reasoning(id="block1", text="first")
+        c2 = Content.from_text_reasoning(id="block2", text="second")
+
+        events1 = _emit_text_reasoning(c1, flow)
+        events2 = _emit_text_reasoning(c2, flow)
+        close = _close_reasoning_block(flow)
+
+        # events1: Start(block1) + MsgStart(block1) + Content(block1)
+        assert events1[0].message_id == "block1"
+        # events2: MsgEnd(block1) + End(block1) + Start(block2) + MsgStart(block2) + Content(block2)
+        assert isinstance(events2[0], ReasoningMessageEndEvent)
+        assert events2[0].message_id == "block1"
+        assert isinstance(events2[1], ReasoningEndEvent)
+        assert events2[1].message_id == "block1"
+        assert isinstance(events2[2], ReasoningStartEvent)
+        assert events2[2].message_id == "block2"
+        # close: MsgEnd(block2) + End(block2)
+        assert isinstance(close[0], ReasoningMessageEndEvent)
+        assert close[0].message_id == "block2"