Skip to content

Conversation

@IdkwhatImD0ing
Copy link
Contributor

@IdkwhatImD0ing IdkwhatImD0ing commented Oct 1, 2025

This pull request includes several improvements and bug fixes related to reasoning summary streaming, message display, and developer tooling for task messages. The most significant changes are in the handling of reasoning summary events in the OpenAI provider, as well as enhancements to the developer message printing utilities.

Reasoning Summary Streaming Improvements:

  • Refactored the handling of reasoning summary streaming in run_agent_streamed_auto_send to use new event types (ResponseReasoningSummaryPartAddedEvent, ResponseReasoningSummaryTextDeltaEvent, ResponseReasoningSummaryPartDoneEvent). Now, reasoning summaries are accumulated as a string and the streaming context is properly managed and closed when the summary part is done, resulting in more accurate and robust streaming of reasoning summaries. [1] [2] [3] [4] [5]

Developer Tooling and Message Display:

  • Improved the print_task_message function to better handle empty reasoning messages, preventing unnecessary output, and adjusted the logic to avoid errors when content is None.
  • Enhanced message display in rich mode: reasoning messages now have a magenta border and author color for clearer visual distinction.
  • Ensured a clean line is printed after spinner stops in subscribe_to_async_task_messages, improving terminal output readability. [1] [2]

Code Cleanups and Maintenance:

  • Updated imports in openai.py to remove unused event classes and include new ones relevant to the revised reasoning summary streaming logic. [1] [2]
  • Removed an unnecessary logger statement in on_task_event_send for cleaner log output.
  • Commented out an obsolete MCP server definition in workflow.py as it is no longer required.

@smoreinis
Copy link
Contributor

This looks reasonable to me.
We just recently got our lint checks passing (yesterday), can we avoid introducing lint errors here?
For my own understanding, is refactoring the handling of reasoning summary streaming (and specifically using new types) also part of the bugfix? Or just an improvement that got rolled into this?

Copy link
Collaborator

@jasonyang101 jasonyang101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stamped but would not modify the ipynb

@IdkwhatImD0ing
Copy link
Contributor Author

This looks reasonable to me. We just recently got our lint checks passing (yesterday), can we avoid introducing lint errors here? For my own understanding, is refactoring the handling of reasoning summary streaming (and specifically using new types) also part of the bugfix? Or just an improvement that got rolled into this?

Basically the past implementation kept streaming contexts open, and did not send full/done events back after each reasoning stream.
This fix correctly sends the done event, and also closes the streaming context properly.

@smoreinis
Copy link
Contributor

This looks reasonable to me. We just recently got our lint checks passing (yesterday), can we avoid introducing lint errors here? For my own understanding, is refactoring the handling of reasoning summary streaming (and specifically using new types) also part of the bugfix? Or just an improvement that got rolled into this?

Basically the past implementation kept streaming contexts open, and did not send full/done events back after each reasoning stream. This fix correctly sends the done event, and also closes the streaming context properly.

Thanks, the part I wasn't clear about was whether or not closing the streaming context properly (and correctly sending the done event) actually required using the new event types or if it would have been possible with the existing ones.

@IdkwhatImD0ing
Copy link
Contributor Author

This looks reasonable to me. We just recently got our lint checks passing (yesterday), can we avoid introducing lint errors here? For my own understanding, is refactoring the handling of reasoning summary streaming (and specifically using new types) also part of the bugfix? Or just an improvement that got rolled into this?

Basically the past implementation kept streaming contexts open, and did not send full/done events back after each reasoning stream. This fix correctly sends the done event, and also closes the streaming context properly.

Thanks, the part I wasn't clear about was whether or not closing the streaming context properly (and correctly sending the done event) actually required using the new event types or if it would have been possible with the existing ones.

Yep, so regarding the new event types
i removed two event types: ResponseReasoningTextDeltaEvent and ResponseReasoningTextDoneEvent due to the openai api actually not sending those events, not sure why they are included in the sdk

I added two events, ResponseReasoningSummaryPartDoneEvent, and and ResponseReasoningSummaryPartAddedEvent

Cause that lets us know when a reasoning chunk is started, and when it ends. That way it lets us correctly send the done event when a reasoning chunk is finished

The flow is

PartAdded
Delta
PartDone

PartAdded
Delta
PartDone

the reason we had to use the part added event is because if we just listen for dones, the stream would be considered closed for the consecutive reasoning chunk, and we couldnt keep sending deltas

so for each chunk we create a new streaming context and close it

@IdkwhatImD0ing IdkwhatImD0ing merged commit f39614b into main Oct 2, 2025
10 checks passed
@IdkwhatImD0ing IdkwhatImD0ing deleted the bill/reasoning_streaming_bugfix branch October 2, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants