fix(simplex): chunk long outbound messages in send() - #35558
Conversation
send() advertised max_message_length but passed full content straight through in a single daemon command, unlike sibling text adapters which self-chunk via the base truncate_message helper. Split long content into <= MAX_MESSAGE_LENGTH chunks via truncate_message and send each in order with the contact/group prefix, in both send() and the out-of-process _standalone_send cron path. Short messages still produce a single send. In _standalone_send the per-chunk corrId now enumerates the loop (hermes-snd-<ms>-<i>) so same-millisecond chunks get distinct correlation ids. Add focused tests: short content -> one send; long content -> multiple ordered sends, each within the limit, with the part-bodies reassembling word-for-word to the original (no content dropped).
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review Findings
This PR adds message chunking for long outbound SimpleX messages — bringing it into line with every other text adapter (signal, telegram, IRC, mattermost, discord) that uses BasePlatformAdapter.truncate_message().
✅ Looks Good
- Correctness: Uses the same
self.truncate_message(content, MAX_MESSAGE_LENGTH)pattern as the mattermost adapter — proven convention. - Code fence preservation: The base
truncate_messagehelper already handles code-block splitting across boundaries. - Part indicators: Trailing
(i/N)markers inform the user of multi-part messages. - Both paths updated: Both
send()(in-process) and_standalone_send()(cron) get chunking. The cron path adds per-chunk enumeratedcorrIdto avoid collision — good thinking. - Clean refactor: Factoring
prefix = f"@[{chat_id}] "/prefix = f"#[{chat_id[6:]}] "out of the loop eliminates repetition. - Tests:
test_send_short_content_single_sendverifies no regression for short messages.test_send_long_content_chunks_into_ordered_sendsprovides thorough coverage — verifies chunk count, per-chunk length, ordered(i/N)markers, and word-for-word content reassembly. - Merge conflict noted: The PR body transparently documents the overlap with #27978 and how the two patches differ — good engineering communication.
No Issues Found
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
Fixes SimpleX long outbound messages causing issues by implementing length-based chunking, matching the pattern used by Signal, Telegram, IRC, and Mattermost.
✅ Looks Good
- Correct approach: Uses existing
BasePlatformAdapter.truncate_message()helper — same pattern as siblings. - Good test coverage: Short messages still send once; long messages produce ordered chunks with content integrity verification.
- Preserves code-block fences: The helper splits without breaking markdown code blocks.
- Well-documented overlap: Acknowledges merge conflict with #27978 and explains how they're complementary.
Reviewed by Hermes Agent (cron job)
822ab38 to
b50b353
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the missing adapter-level chunking; current main still sends full content through SimplexAdapter.send() (plugins/platforms/simplex/adapter.py:831-843).
Problems
- This branch uses the superseded
@[contact]and#[group]command forms (plugins/platforms/simplex/adapter.py:511-513,:646-648). Current main uses@<id>for DMs and structured/_send #<id> jsonfor groups (plugins/platforms/simplex/adapter.py:831-843,:1193-1212), so the conflicting patch cannot be applied directly. - Native chunking also needs
splits_long_messages = True; otherwisegateway/delivery.py:403-451truncates oversized cron output before it reaches the adapter. - Current main strips
MEDIA:tags and dispatches native attachments insend()(plugins/platforms/simplex/adapter.py:826-852). Please integrate chunking after that extraction rather than replacing the older send implementation.
Suggested changes
- Format each chunk with current main's DM/group command paths and add coverage for structured group chunks, standalone delivery, and media-tag preservation.
Automated hermes-sweeper review.
| if chat_id.startswith("group:"): | ||
| group_id = chat_id[6:] | ||
| cmd_str = f"#[{group_id}] {content}" | ||
| prefix = f"#[{chat_id[6:]}] " |
There was a problem hiding this comment.
Current main replaced this bracket group command with structured /_send #<id> json because #[<id>] is parsed as a display-name lookup and can silently miss the intended group. Preserve that current group formatter and emit one JSON payload per chunk instead.
| "corrId": corr_id, | ||
| "cmd": cmd_str, | ||
| } | ||
| for chunk in self.truncate_message(content, MAX_MESSAGE_LENGTH): |
There was a problem hiding this comment.
Please also set splits_long_messages = True on SimplexAdapter. gateway/delivery.py:403-451 otherwise treats SimpleX as non-chunking and truncates oversized cron output to 4,000 characters before this loop runs.
Summary
The SimpleX adapter advertises
max_message_length=MAX_MESSAGE_LENGTH(16,000), butsend()passed the fullcontentstraight through to the daemon in a single command — unlike every other text adapter (signal/telegram/irc/mattermost/discord), which self-chunk via the basetruncate_messagehelper.This change makes
send()(and the out-of-process_standalone_sendcron path) split long content into<= MAX_MESSAGE_LENGTHchunks using the same baseBasePlatformAdapter.truncate_message(content, max_length)helper the siblings use, sending each chunk in order with the contact/group address prefix. The helper preserves code-block fences across splits and appends(i/N)part indicators. Short messages still produce exactly one send — no behavior change.The mattermost adapter's
send()is the convention followed here:for chunk in self.truncate_message(formatted, MAX): ....Tests
tests/gateway/test_simplex_plugin.py:test_send_short_content_single_send— content within the limit yields exactly one WS send.test_send_long_content_chunks_into_ordered_sends— content longer than the max yields multiple ordered sends, each chunk within the limit, each carrying the contact prefix and a trailing(i/N)indicator (matched by regex, tolerant of the helper's spacing), and — the meaningful coverage check — the indicator-stripped chunk bodies reassemble word-for-word to the original message, so no content can be silently dropped.Both fail to detect chunking on the unmodified adapter (the long-content test asserts
>1send and is red on clean main) and pass with the change.Overlap with #27978
Heavy region overlap with PR #27978 (groups, native attachments, text batching, auto-accept), which rewrites both
send()and_standalone_send— the two functions touched here — so a textual merge conflict is guaranteed if both land. But despite its "text batching" title, #27978 does not implement length-based chunking: its rewrittensend()extractsMEDIA:tags and switches groups to the structured/_send #<id> jsonform, but still passes the full text body through unchunked, and it lowersMAX_MESSAGE_LENGTHto 8000 without enforcing it. So this is functionally complementary, not a duplicate.For whoever merges second: folding this chunking loop into #27978's group path is not a drop-in — #27978 sends groups via the structured
/_send #<id> jsoncommand, so it would need a separatejson.dumps-encoded payload per chunk (one structured message object per part), not the simpleprefix + chunkconcatenation used for the direct-contact path here. The direct-contact@[id] <chunk>path folds in cleanly. Flagging it so it isn't a surprise at merge time.Internals tidy: in
_standalone_sendthe per-chunkcorrIdenumerates the loop (hermes-snd-<ms>-<i>) so chunks emitted within the same millisecond no longer share a correlation id; the in-processsend()already usesself._make_corr_id(), which appends a random suffix.