fix: close orphaned code fences on all send paths - #48476
Conversation
- A/B/C: truncate_message with reasoning format, carry_lang, multiple blocks - D: last-chunk early-break gap - E: _filter_and_accumulate preserves triple-backtick outside think blocks - F: _split_text_chunks has no fence tracking - G: reasoning truncation (short pass-through, long gap) - H: stream consumer accumulator has no fence state - I: fix stub - J: edit path bypasses truncate_message entirely - K: overflow split first chunk via edit path - L: fallback final + split_text_chunks noop Four gaps identified: G1: truncate_message last-chunk early-break path G2: streaming edit path bypasses truncate_message G3: overflow split first chunk via edit path G4: split_text_chunks + truncate_message no-op
Adds `ensure_closed_code_fences()` helper to detect text with an odd count of triple-backtick markers (indicating an unclosed code block) and append a closing fence. Applies the fix to all four identified gap paths: G1: truncate_message early-break path for final chunk G2: _send_or_edit streaming edit path (most commonly hit) G3: overflow split first chunk (covered by G2's fix) G4: _send_fallback_final fallback send path Closes: #TBD
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing markdown rendering issue in messaging gateways where truncated model output can leave an orphaned triple-backtick fence, causing Discord/Slack/Matrix to render the remainder of the message as one giant code block.
Changes:
- Added
ensure_closed_code_fences()and applied it to streaming send/edit and fallback-final delivery paths ingateway/stream_consumer.py. - Updated
BasePlatformAdapter.truncate_message()to close orphaned fences on the final chunk (previously only intermediate chunks were guaranteed to be balanced). - Added a dedicated gateway test module covering fence behavior across truncation/splitting/streaming paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
gateway/stream_consumer.py |
Adds a helper to close orphaned ``` fences and applies it to key streaming/fallback send paths. |
gateway/platforms/base.py |
Extends truncate_message() to fence-balance the final chunk (previously an early-break edge case). |
tests/gateway/test_code_fence_tracking.py |
Introduces tests documenting/validating code-fence behavior across gateway delivery paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Known gap: truncate_message closes orphaned fences on INTERMEDIATE chunks but | ||
| NOT on the FINAL chunk (line 4853-4854: ``if _len(prefix) + _len(remaining) | ||
| <= max_length - INDICATOR_RESERVE: chunks.append(prefix + remaining); break`` | ||
| skips the fence-closing check that intermediate chunks get at line 4904-4922). | ||
|
|
| class TestFixStub: | ||
| """If a _ensure_closed_code_fences() helper were added, these tests | ||
| should pass.""" | ||
|
|
||
| @staticmethod | ||
| def _ensure_closed_fences(text: str) -> str: | ||
| """Stub: append closing ``` if odd count and text doesn't already | ||
| end with a closing fence on its own line.""" | ||
| if _odd_fences(text): | ||
| return text.rstrip() + "\n```" | ||
| return text | ||
|
|
||
| def test_closes_unclosed(self): | ||
| assert not _odd_fences(self._ensure_closed_fences( | ||
| "💭 **Reasoning:**\n```\ncut off" | ||
| )) | ||
|
|
||
| def test_noop_balanced(self): | ||
| t = "```\nblock\n```\ncontent" | ||
| assert self._ensure_closed_fences(t) == t | ||
|
|
||
| def test_noop_no_fence(self): | ||
| t = "plain text" | ||
| assert self._ensure_closed_fences(t) == t | ||
|
|
||
| def test_noop_already_ends_with_close(self): | ||
| t = "```\nblock\n```" | ||
| assert self._ensure_closed_fences(t) == t | ||
|
|
| class TestTruncateMessageLastChunkGap: | ||
| """The final chunk (when ``remaining`` fits) is appended via the | ||
| early-break path at line 4853-4854 of base.py, which does NOT run | ||
| the fence-balance check. If the remaining content has an odd count | ||
| of ```, so does the final chunk.""" | ||
|
|
||
| def test_last_chunk_can_have_odd_fence_when_content_unclosed(self): | ||
| """Content with unclosed ``` where the last chunk fits → no fix.""" | ||
| long_body = "\n".join(f"line{i}" for i in range(100)) | ||
| content = f"```\n{long_body}" | ||
| # The first split happens at ~186 chars, last chunk is small | ||
| chunks = BasePlatformAdapter.truncate_message(content, 150) | ||
| assert len(chunks) >= 2 | ||
| # The last chunk may have odd ``` because the remaining content | ||
| # (after carry_lang prefix) doesn't contain a closing ``` | ||
| last = chunks[-1] | ||
| # Strip the (N/N) indicator | ||
| last_clean = last.rsplit(" (", 1)[0] | ||
| if _odd_fences(last_clean): | ||
| # This demonstrates the GAP — last chunk has unbalanced fence | ||
| pass # Not asserting — the gap is real | ||
|
|
| Note: This class uses synchronous tests because pytest-asyncio is not | ||
| installed in this project (existing stream consumer tests use it but | ||
| the conftest may register the marker differently). We test the | ||
| accumulator behaviour directly. |
| # Everything remaining fits in one final chunk | ||
| if _len(prefix) + _len(remaining) <= max_length - INDICATOR_RESERVE: | ||
| chunks.append(prefix + remaining) | ||
| final_chunk = prefix + remaining | ||
| # Check fence balance: if carry_lang was set, the chunk | ||
| # starts with an opening fence. Walk the remaining text | ||
| # to see if the code block was closed; if not, close it. | ||
| _final_in_code = carry_lang is not None | ||
| _final_lang = carry_lang or "" | ||
| if _final_in_code: | ||
| for _line in remaining.split("\n"): | ||
| _stripped = _line.strip() | ||
| if _stripped.startswith("```"): |
ensure_closed_code_fences previously only handled triple-backtick (```) code-block fences. Single backtick (`) inline-code spans have the same problem: an orphaned opening backtick causes the remainder of the message to render as inline code on Discord and other platforms. After balancing triple-backtick fences, strip complete ```...``` regions and count remaining standalone backtick markers. If odd, append a closing backtick. Same trade-off as the triple-backtick fix: a stray closing backtick may create a brief empty inline-code span, which is far less harmful than the rest of the message being inline code.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the streaming and fallback delivery paths. The underlying bug remains on current main: _send_or_edit() cleans then delivers the text without balancing fences (gateway/stream_consumer.py:1647, 1771-1775), while fallback splitting is fence-unaware (gateway/stream_consumer.py:965-983).
Problems
- Balancing only the outbound edit changes
_last_sent_text(gateway/stream_consumer.py:1804) but not_accumulated. Fallback dedup only removes a prefix whenfinal_text.startswith(prefix)(gateway/stream_consumer.py:957-962), so a synthetic closing fence can turn a real prefix into a mismatch and resend the full response. - The raw overflow split remains at
gateway/stream_consumer.py:713-737. Closing its first fragment without reopening the next fragment loses code-fence continuity. Linked PR #48500 identifies this same sibling path. - The added suite includes non-regression coverage:
tests/gateway/test_code_fence_tracking.py:193permits either outcome, and existing review comment 3436875418 notes a local stub does not exercise the production helper.
Suggested changes
- Preserve canonical accumulation/prefix semantics while rendering temporary fence closures.
- Use a shared close-and-reopen splitter for overflow and fallback paths, with async send/edit/fallback regression tests.
- Replace stale and tautological tests with production-path assertions.
Automated hermes-sweeper review.
| if _odd_fences(last_clean): | ||
| # This demonstrates the GAP — last chunk has unbalanced fence | ||
| pass # Not asserting — the gap is real | ||
|
|
There was a problem hiding this comment.
This accepts both possible parity values, so it cannot detect a regression. Assert the required post-fix fence state instead; the current base adapter test suite already models every emitted chunk as balanced.
Widen #48476's fence guarantees to the two splitters that still emitted fence-broken chunks: * GatewayStreamConsumer._split_text_chunks (fallback final send): close the orphaned ``` at each chunk boundary and reopen it — with the original language tag — on the next chunk, mirroring BasePlatformAdapter.truncate_message's contract. Headroom is reserved so balanced chunks stay within the platform limit. * Slack block_kit._split_text (3000-char section chunking): same close/reopen balancing for mrkdwn section text carrying fences. With these, every chunk boundary — non-streaming send (truncate_message), streaming overflow (_truncate_for_stream via adapter.truncate_message per #45938), fallback final (_split_text_chunks), final-send balance (ensure_closed_code_fences), and Block Kit section splits — delivers fence-balanced chunks. Regression tests probe each path with fenced fixtures, assert per-chunk balance, limit compliance, language-tag reopening, and prose passthrough.
Widen #48476's fence guarantees to the two splitters that still emitted fence-broken chunks: * GatewayStreamConsumer._split_text_chunks (fallback final send): close the orphaned ``` at each chunk boundary and reopen it — with the original language tag — on the next chunk, mirroring BasePlatformAdapter.truncate_message's contract. Headroom is reserved so balanced chunks stay within the platform limit. * Slack block_kit._split_text (3000-char section chunking): same close/reopen balancing for mrkdwn section text carrying fences. With these, every chunk boundary — non-streaming send (truncate_message), streaming overflow (_truncate_for_stream via adapter.truncate_message per #45938), fallback final (_split_text_chunks), final-send balance (ensure_closed_code_fences), and Block Kit section splits — delivers fence-balanced chunks. Regression tests probe each path with fenced fixtures, assert per-chunk balance, limit compliance, language-tag reopening, and prose passthrough.
Widen #48476's fence guarantees to the two splitters that still emitted fence-broken chunks: * GatewayStreamConsumer._split_text_chunks (fallback final send): close the orphaned ``` at each chunk boundary and reopen it — with the original language tag — on the next chunk, mirroring BasePlatformAdapter.truncate_message's contract. Headroom is reserved so balanced chunks stay within the platform limit. * Slack block_kit._split_text (3000-char section chunking): same close/reopen balancing for mrkdwn section text carrying fences. With these, every chunk boundary — non-streaming send (truncate_message), streaming overflow (_truncate_for_stream via adapter.truncate_message per #45938), fallback final (_split_text_chunks), final-send balance (ensure_closed_code_fences), and Block Kit section splits — delivers fence-balanced chunks. Regression tests probe each path with fenced fixtures, assert per-chunk balance, limit compliance, language-tag reopening, and prose passthrough.
|
Merged via #70191 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your fence-closing on all send paths was cherry-picked (all 3 commits). Thanks for the contribution! |
Widen NousResearch#48476's fence guarantees to the two splitters that still emitted fence-broken chunks: * GatewayStreamConsumer._split_text_chunks (fallback final send): close the orphaned ``` at each chunk boundary and reopen it — with the original language tag — on the next chunk, mirroring BasePlatformAdapter.truncate_message's contract. Headroom is reserved so balanced chunks stay within the platform limit. * Slack block_kit._split_text (3000-char section chunking): same close/reopen balancing for mrkdwn section text carrying fences. With these, every chunk boundary — non-streaming send (truncate_message), streaming overflow (_truncate_for_stream via adapter.truncate_message per NousResearch#45938), fallback final (_split_text_chunks), final-send balance (ensure_closed_code_fences), and Block Kit section splits — delivers fence-balanced chunks. Regression tests probe each path with fenced fixtures, assert per-chunk balance, limit compliance, language-tag reopening, and prose passthrough.
Summary
When model output is truncated mid-code-block (e.g. by token limits or a finish_reason="length"), the resulting message has an unclosed code fence (
```). On Discord, Slack, and Matrix this causes everything after the orphaned fence to render as a single giant code block.Adds
ensure_closed_code_fences()helper that detects an odd count of triple-backtick markers and appends a closing fence. Applies it to all four identified gap paths ingateway/stream_consumer.py:truncate_messageearly-break path for final chunk_send_or_editstreaming edit path (most commonly hit)_send_fallback_finalfallback send pathTesting
26 tests covering: truncation at fence boundaries, split-text chunks, fallback paths, edit-path bypass, and edge cases (balanced fences, no fences, consecutive fence markers).