fix(gateway): finish the split-delivery bug class behind the swallowed-finals fix (salvages #78558) - #79669
Merged
kshitijk4poor merged 3 commits intoAug 5, 2026
Conversation
Record an unsplit stream ledger for multi-message deliveries and refuse legacy trust when split delivery left no payload, so Telegram group sessions no longer suppress a complete reply after an early/partial finalize (NousResearch#78541).
Add unit and GatewayRunner boundary coverage for the NousResearch#78541 shape where final_content_delivered is set via split delivery with no recorded payload.
…uplicate or still swallow The salvaged fix changed only the gateway's verdict: a payload-less multi-message split stopped inheriting legacy trust. But six code paths set _turn_split_delivery, and only one of them was taught to record a payload, so the remaining five swapped the swallow for the opposite defect. Fix the producers instead of only distrusting them at the boundary: - _send_or_edit failed-final-edit branch: record the visible payload on split turns too. It deliberately skipped recording, which now reads as a mismatch and re-sends an answer already on screen -- reintroducing the duplicate NousResearch#45517 fixed (NousResearch#36965 / NousResearch#25349). - _send_fallback_final (x2) and _send_empty_fallback_final: route through _record_turn_final_payload instead of assigning _delivered_final_text directly. On a split turn their final_text is only the trailing chunk, so a fully delivered heads+tail reply recorded a tail-only payload and was re-sent in full. - _try_fresh_final: refuse the fresh-final route once a head chunk is sealed. It replaces every tracked preview with one message, which only holds the whole answer on a single-message turn. After a split it deleted the sealed heads while sending just the tail, so the complete reply was still lost -- on Telegram, the default finalize route and the shape NousResearch#78541 reports. - Set _turn_split_delivery at seal time rather than after the tail send, so the tail's own finalize sees the split state. The sibling overflow path already did this; the divergence is what let fresh-final delete the heads. - run.py stale-finalize reconciliation: skip the in-place edit on a split delivery. message_id is only the LAST chunk there, so editing it with the complete response repeated every sealed head's text inside the tail message. Fall through to the normal final send. Also drop a dead `or "".join(chunks)` fallback (all growth funnels through _append_accumulated, so the ledger is never empty at that call site, and joined chunks carry injected fence markers that could never match final_response), and document that _record_turn_final_payload intentionally ignores its argument on split turns. Tests: four end-to-end cases driving the real overflow-split loop instead of hand-setting private flags -- complete split still suppresses (no duplicate), split missing a tail does not suppress, fresh-final keeps sealed heads, and a flood-controlled final edit after a split stays suppressed. Each was mutation-checked: reverting any individual fix turns its test red. The pre-existing gateway-boundary test asserted the recovery *route* (the reconcile edit) rather than the guarantee. Relaxed to the real contract: either _run_agent puts the complete text on the wire, or it declines to claim delivery so the caller's normal final send does. Co-authored-by: HexLab98 <liruixinch@outlook.com>
This was referenced Aug 5, 2026
kshitijk4poor
added a commit
that referenced
this pull request
Aug 5, 2026
…ty-fallback recovery Follow-up to #79669. That PR routed the three fallback recorder sites through _record_turn_final_payload so a split turn would record the unsplit ledger instead of a tail-only payload. For two of them that is right. For _send_empty_fallback_final it is wrong, and it reintroduces the #78541 swallow at the one site that was supposed to be fixed. _send_empty_fallback_final is a *replacement* recovery: it sends the completed text as a fresh message and deletes every tracked segment preview -- which on an overflow split includes the sealed head chunks. After it runs, the only thing on screen is the message it just sent. Recording the ledger there claims delivery for text the same function just removed, so delivered_final_matches() returns True, the gateway suppresses its own send, and the user is left with a fraction of the answer. Observed with a probe driving the real run() loop (543-char reply, 475-char head sealed then deleted, 67-char tail committed): before this fix recorded=543 matches=True -> suppressed, 67/543 on screen after this fix recorded=67 matches=False -> gateway sends the full answer Record final_text verbatim here instead. The sibling site in _send_fallback_final keeps the recorder: its delete is gated on `continuation == final_text` and targets only the single active partial, never the sealed heads, so the ledger correctly describes what survives. The distinction is whether a recovery ADDS to what is on screen or REPLACES it. Additive paths may record the ledger; replacing paths must record only what they leave behind. _try_fresh_final is the same shape and #79669 handled it by refusing the route on split turns. Test drives the real seal-then-delete sequence and asserts the mismatch, so the gateway is required to re-send. Mutation-checked: restoring the recorder call turns it red.
randlee
pushed a commit
to randlee/hermes-agent
that referenced
this pull request
Aug 11, 2026
…ty-fallback recovery Follow-up to NousResearch#79669. That PR routed the three fallback recorder sites through _record_turn_final_payload so a split turn would record the unsplit ledger instead of a tail-only payload. For two of them that is right. For _send_empty_fallback_final it is wrong, and it reintroduces the NousResearch#78541 swallow at the one site that was supposed to be fixed. _send_empty_fallback_final is a *replacement* recovery: it sends the completed text as a fresh message and deletes every tracked segment preview -- which on an overflow split includes the sealed head chunks. After it runs, the only thing on screen is the message it just sent. Recording the ledger there claims delivery for text the same function just removed, so delivered_final_matches() returns True, the gateway suppresses its own send, and the user is left with a fraction of the answer. Observed with a probe driving the real run() loop (543-char reply, 475-char head sealed then deleted, 67-char tail committed): before this fix recorded=543 matches=True -> suppressed, 67/543 on screen after this fix recorded=67 matches=False -> gateway sends the full answer Record final_text verbatim here instead. The sibling site in _send_fallback_final keeps the recorder: its delete is gated on `continuation == final_text` and targets only the single active partial, never the sealed heads, so the ledger correctly describes what survives. The distinction is whether a recovery ADDS to what is on screen or REPLACES it. Additive paths may record the ledger; replacing paths must record only what they leave behind. _try_fresh_final is the same shape and NousResearch#79669 handled it by refusing the route on split turns. Test drives the real seal-then-delete sequence and asserts the mismatch, so the gateway is required to re-send. Mutation-checked: restoring the recorder call turns it red.
33hodl
pushed a commit
to 33hodl/hermes-agent
that referenced
this pull request
Aug 12, 2026
…ty-fallback recovery Follow-up to NousResearch#79669. That PR routed the three fallback recorder sites through _record_turn_final_payload so a split turn would record the unsplit ledger instead of a tail-only payload. For two of them that is right. For _send_empty_fallback_final it is wrong, and it reintroduces the NousResearch#78541 swallow at the one site that was supposed to be fixed. _send_empty_fallback_final is a *replacement* recovery: it sends the completed text as a fresh message and deletes every tracked segment preview -- which on an overflow split includes the sealed head chunks. After it runs, the only thing on screen is the message it just sent. Recording the ledger there claims delivery for text the same function just removed, so delivered_final_matches() returns True, the gateway suppresses its own send, and the user is left with a fraction of the answer. Observed with a probe driving the real run() loop (543-char reply, 475-char head sealed then deleted, 67-char tail committed): before this fix recorded=543 matches=True -> suppressed, 67/543 on screen after this fix recorded=67 matches=False -> gateway sends the full answer Record final_text verbatim here instead. The sibling site in _send_fallback_final keeps the recorder: its delete is gated on `continuation == final_text` and targets only the single active partial, never the sealed heads, so the ledger correctly describes what survives. The distinction is whether a recovery ADDS to what is on screen or REPLACES it. Additive paths may record the ledger; replacing paths must record only what they leave behind. _try_fresh_final is the same shape and NousResearch#79669 handled it by refusing the route on split turns. Test drives the real seal-then-delete sequence and asserts the mismatch, so the gateway is required to re-send. Mutation-checked: restoring the recorder call turns it red.
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
Salvages #78558 (@HexLab98) onto current
mainand finishes the bug class it opened.The original fix is correct about the cause: on a multi-message split delivery the stream consumer set
final_content_delivered=Truewithout recording a payload,delivered_final_matches()returnedNone, andgateway/run.pytreatedNoneas "trust the flag" — so the completed reply was suppressed and never sent. That's #78541: 165 suppressed events with 100–3000 char replies lost over 48h across three Telegram groups.But six code paths set
_turn_split_delivery, and only one was taught to record a payload. The other five inherited the new "payload-less split ⇒ mismatch" verdict without being able to satisfy it, which swapped the swallow for the opposite defect. This PR fixes the producers rather than only distrusting them at the boundary.Context — what actually changed for a user
Before (on
main): a long Telegram group reply that overflows into several messages, where the stream ends before the tail arrives. Log shows the reply is ready, and it is never sent:The 1939-char answer never reaches the chat. Reproduced with a probe driving the real
run()loop:What #78558 alone would have shipped. Same probe harness, three shapes it does not cover:
The duplicate on row 2 is the regression that matters most: it re-opens the double-answer #45517 fixed (#36965 / #25349). Row 4 is why the reported symptom would likely have persisted —
_try_fresh_finalis Telegram's default finalize route.Changes
Kept from #78558 (@HexLab98's two commits, authorship preserved):
_stream_ledger(an un-truncated mirror of_accumulated, since the overflow paths truncate_accumulatedto the tail once heads are sealed), theFalse-on-payload-less-split verdict, and the tri-state reordering. Confirmed the ledger is genuinely load-bearing — nothing else records the union of a multi-message delivery:_delivered_segment_textsis only appended in_reset_segment_state(which the split paths never reach; they clear_last_sent_textfirst), andhas_delivered_textcompares whole-string equality per entry, so it structurally cannot matchchunk1 + chunk2.Added on top:
_send_or_editfailed-final-edit branch — record the visible payload on split turns too. It deliberately skipped recording, which now reads as a mismatch and re-sends an answer already on screen._send_fallback_final(×2) +_send_empty_fallback_final— route through_record_turn_final_payloadinstead of assigning_delivered_final_textdirectly. On a split turn theirfinal_textis only the trailing chunk, so a fully delivered heads+tail reply recorded a tail-only payload and was re-sent in full._try_fresh_final— refuse the fresh-final route once a head chunk is sealed. It replaces every tracked preview with one message, which only holds the whole answer on a single-message turn; after a split it deleted the sealed heads while sending just the tail._turn_split_deliveryat seal time rather than after the tail send, so the tail's own finalize sees the split state. The sibling overflow path already did this — that divergence is what let fresh-final delete the heads.run.pystale-finalize reconciliation — skip the in-place edit on a split delivery.message_idis only the last chunk there, so editing it with the complete response repeated every sealed head's text inside the tail message. Falls through to the normal final send.or "".join(chunks)fallback (all growth funnels through_append_accumulated, so the ledger is never empty at that call site; and joined chunks carry injected fence markers that could never equalfinal_response), and documented that_record_turn_final_payloadintentionally ignores its argument on split turns.Validation
tests/gateway/test_stale_finalize_suppression.pytests/gateway/ -k 'stream or consumer or final or deliver or suppress or overflow or split or telegram'mautrixmock skip)py_compileon all three filesmain, realrun()loopMutation-checked — reverting any individual fix turns its own test red, so none of the new tests are vacuous:
_try_fresh_finalsplit guardtest_split_delivery_keeps_sealed_heads_on_fresh_finalfailstest_failed_final_edit_after_split_records_visible_payloadfailsdelivered_final_matchesto pre-PRFour new tests drive the real overflow-split loop rather than hand-setting private flags: complete split still suppresses (no duplicate), split missing a tail does not suppress, fresh-final keeps sealed heads, and a flood-controlled final edit after a split stays suppressed.
One pre-existing assertion relaxed: the gateway-boundary test asserted the recovery route (that
_run_agentperforms the reconcile edit) rather than the guarantee. Since a split delivery now deliberately skips that edit, it asserts the real contract — either_run_agentputs the complete text on the wire, or it declines to claim delivery so the caller's normal final send does. Verified the latter holds:already_sentcomes back falsy.Credit
@HexLab98 diagnosed the root cause and wrote the ledger mechanism; both commits are preserved with authorship intact.
Also closes #78556 (@686f6c61), which fixed the same issue independently. Its approach records
"".join(chunks[:-1]) + _accumulated, but the splitter strips the newline separators (.lstrip("\n")) and injects fence markers at boundaries, so the join can never equalfinal_response— verified it returnsFalseon every complete overflow split, i.e. duplicate sends on all long replies. Credited for independent diagnosis.Closes #78541