fix(gateway): finalize oversize-split stream chunk so it gets final platform markup - #38971
Closed
GodsBoy wants to merge 1 commit into
Closed
fix(gateway): finalize oversize-split stream chunk so it gets final platform markup#38971GodsBoy wants to merge 1 commit into
GodsBoy wants to merge 1 commit into
Conversation
… turn-final When a mid-stream message exceeds the platform safe limit, the first half is sent and then _message_id is reset to None, so that chunk can never be edited again. It was sent with the default finalize=False, leaving it stuck without the adapter's final rich-text markup (raw MarkdownV2 on Telegram). Pass finalize=True so the chunk receives its final platform formatting before it is surrendered, and is_turn_final=False so _try_fresh_final does not set _final_response_sent on this mid-stream chunk. Marking it as the turn-final answer would let a cancel/timeout before got_done suppress the real final send and strand the continuation (the remainder is still the real answer). Adds two regression tests: a unit check that the _send_or_edit contract keeps _final_response_sent False under is_turn_final=False, and an end-to-end run-loop test that drives the overflow branch and guards the call site argument.
Contributor
|
Thanks for isolating the overflow-finalization behavior and preserving the non-turn-final flag. Automated hermes-sweeper review found this exact behavior is already on
Closing as already implemented on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a streamed message grows past the platform safe limit mid-stream, the consumer splits it: the first chunk is sent and then
_message_idis reset toNoneso the remainder starts a fresh message. That first chunk was sent with the defaultfinalize=False, so it never received the adapter's final rich-text markup (raw MarkdownV2 on Telegram) and could never be edited again.This sends the split chunk with
finalize=Trueso it gets its final platform formatting before it is surrendered, and withis_turn_final=Falseso it is not mistaken for the turn-final answer.Why
is_turn_final=FalsemattersThe split chunk enters the
_message_id is not Noneedit branch, wherefinalize=Truecan trigger_try_fresh_final. That path is enabled by default on Telegram (fresh_final_after_seconds=60). If it fired with the defaultis_turn_final=True, it would set_final_response_sent=Trueon a mid-stream chunk. A cancel or timeout beforegot_donewould then let the gateway suppress the real final send and strand the continuation (the remainder is still the real answer). The sibling segment-break send already passesis_turn_final=got_donefor the same reason (#29346).Changes
gateway/stream_consumer.py: oversize-split first chunk now sent via_send_or_edit(chunk, finalize=True, is_turn_final=False).tests/gateway/test_stream_consumer_fresh_final.py: two regression tests._send_or_edit(finalize=True, is_turn_final=False)runs fresh-final without setting_final_response_sent.is_turn_final=False. It fails if the call site is reverted to a barefinalize=True.Test plan
Result: 133 passed, 0 failed.
Context
This is the one salvageable change observed in #32609, rebased onto current
mainand hardened with theis_turn_finalfix and regression coverage. The other changes in that PR are already onmain(draft MarkdownV2 parity via #37250, sanitized skip-path logging) or broaden the media-delivery allowlist in a way that bypasses the credential denylist, so they are intentionally not carried here.