fix(telegram): handle 'message is too long' with retry splitting

[Evrard-Nil]

Summary

• Reduce TELEGRAM_MAX_MESSAGE_LEN from 4096 to 4000 for safety margin against Markdown entity/emoji counting edge cases
• Add SendError::TooLong variant to detect "message is too long" 400 responses
• Add send_chunk() helper that recursively halves chunks on TooLong rejections (up to MAX_SPLIT_DEPTH = 2 levels deep, max 4 sub-messages per chunk), splitting at natural boundaries (paragraph > newline > space)
• Use UTF-16 code unit length for split midpoint calculation (matching Telegram's actual limit)
• Pass use_markdown flag through recursion so ParseEntities fallback disables Markdown for all subsequent splits

Fixes the recurring error:

Telegram API returned 400: {"ok":false,"error_code":400,"description":"Bad Request: message is too long"}

Test plan

• All 56 existing + new telegram-channel tests pass
• test_split_message_each_chunk_fits validates against 4000-char limit
• 5 new find_split_midpoint tests (paragraph, newline, space, no-boundary, emoji-heavy)
• Deploy and verify long agent responses no longer fail on Telegram

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR improves Telegram delivery reliability for long agent responses by detecting Telegram’s “message is too long” rejection and retrying with safer chunking/splitting behavior.

Changes:

• Reduced TELEGRAM_MAX_MESSAGE_LEN from 4096 to 4000 to add a safety margin.
• Added SendError::TooLong and detection of “message is too long” 400 responses.
• Refactored text sending to use a new send_chunk() helper that retries on Markdown parse errors and recursively splits on TooLong.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gemini-code-assist]

Code Review

This pull request improves Telegram message handling by reducing the maximum message length to 4000 characters and implementing a recursive splitting mechanism to handle 'message too long' errors from the API. A logic error was identified in the recursive split implementation where the function returns the message ID of the first part of a split instead of the last, which would break the reply chain for subsequent message chunks.

[serrrfirat]

Findings:

1. High: send_chunk_inner() returns first_id after a recursive TooLong split, even when it has sent a second follow-up chunk. send_response() uses the returned id as the next reply_to, so subsequent chunks will reply to the first half rather than the last sent half, breaking the intended linear Telegram thread.
2. Medium: there is no test coverage for the new recursive TooLong path or its return-value contract. The existing split_message tests do not exercise send_chunk_inner() or verify that a recursively split send returns the final sent message id.

Residual risks:

• In the ParseEntities fallback path, a plain-text retry that then hits TooLong still bubbles up as an error instead of being split again.
• Lowering the proactive split threshold from 4096 to 4000 is a user-visible behavior change for messages in the 4001-4096 range; that may be intentional, but it should be treated as such.

Validation: cargo test --lib passed in channels-src/telegram.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[serrrfirat]

Medium Severity — Markdown retry wastes API calls after ParseEntities+TooLong

When the flow is: Markdown → ParseEntities → plain text → TooLong → split_and_send, the recursive send_chunk_inner calls attempt to send sub-chunks with Markdown again (line ~1403 always uses Some("Markdown")). A chunk that already failed Markdown parsing will be retried with Markdown in the split path, potentially failing again with ParseEntities, then falling back to plain text. While this converges (the halves are smaller), it doubles API calls.

Suggested fix: Pass a use_markdown: bool flag through the recursion so that once ParseEntities is hit, all subsequent splits send as plain text directly. Or accept the extra calls as a tradeoff and document it.

---

Medium Severity — No unit tests for core new functions

send_chunk, send_chunk_inner, and split_and_send are the core new functions but have zero unit test coverage. The test plan is manual only.

Suggested fix: Extract the split_and_send splitting logic (pure computation: finding mid_char, split_at, slicing) into a standalone testable function, and add unit tests with edge cases (all-newlines, no-spaces, single-char, exactly-at-boundary).

[serrrfirat]

Paranoid Architect Review — Verdict: 🔄 REQUEST CHANGES (2 High)

⚠️ This PR also has merge conflicts and needs a rebase onto staging.

The PR solves a real production problem (Telegram rejecting messages due to Markdown entity overhead) with a reasonable recursive-halving approach. The recursion depth is bounded and error handling follows existing patterns. However, there's a correctness issue in the splitting logic.

Findings

#	Severity	Category	File	Finding	Suggested Fix
1	High	Correctness	lib.rs split_and_send	UTF-16/chars unit mismatch. Uses chars().count() / 2 for midpoint, but Telegram enforces UTF-16 code unit limits. Existing split_message correctly uses utf16_code_unit_len(). For emoji-heavy messages (common on Telegram), chars().count() underestimates UTF-16 length. Halves may still exceed UTF-16 limit, causing extra recursion or failure at depth 3.	Use utf16_code_unit_len(text) / 2 for midpoint, convert back via prefix_within_utf16_limit()
2	High	Resource	lib.rs	Exponential message count. Depth-3 recursion: up to 8 messages per chunk. With 10 pre-split chunks = up to 80 API calls. Telegram rate limit is ~30 msg/sec per chat.	Add total message count guard or warning log. Consider depth 2 (max 4 sub-messages).
3	Medium	String Safety	lib.rs split_and_send	Split boundary \n\n characters silently discarded via trim_end()/trim_start(). Acceptable but undocumented.	Add comment
4	Medium	Edge Case	lib.rs split_and_send	Empty text: both halves become "", Telegram rejects. Practically unreachable but unguarded.	Add if text.is_empty() { return Err(...) }
5	Medium	Stale Docs	lib.rs:409	Doc comment still says "4096 UTF-16-unit limit" but constant changed to 4000	Update reference
6	Medium	Test Coverage	tests	No tests for send_chunk, send_chunk_inner, or split_and_send. Core behavioral change untested.	Extract find_midpoint_split() as pure function, add unit tests
7	Low	Logging	lib.rs	Old code logged per-chunk success. New code silent on success.	Add debug! in Ok branch
8	Low	Error Quality	lib.rs	Depth-limit error gives no indication recursion exhausted	Wrap: "still too long after splitting N levels deep"
9	Low	Docs	lib.rs	96-char safety margin (4096 to 4000) rationale undocumented	Document worst-case observed or make configurable
10	Nit	Naming	lib.rs	split_and_send describes implementation not intent	Consider halve_and_retry_send

Summary

Finding #1 is the key correctness issue: the new splitting logic uses chars() while the existing code uses UTF-16 code units. For emoji-heavy messages, this mismatch means the retry mechanism can fail to produce small-enough chunks. Finding #2 notes the potential for rate limit issues. Both should be addressed.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

PR description says the recursive too-long splitting retries are "up to 3 levels deep", but the implementation caps recursion at MAX_SPLIT_DEPTH = 2. Please align the constant (or the PR description / docs) so it’s clear what the actual worst-case fan-out is in production.

[Copilot]

split_and_send trims both halves (trim_end / trim_start) before sending, but doesn’t guard against the first half becoming empty (e.g., when splitting text that’s largely whitespace). That can cause an attempt to send an empty message and abort the retry flow. Consider ensuring first is non-empty before calling send_chunk_inner (pick a different split point or skip sending an empty half).

Suggested change

	if first.is_empty() {
	if second.is_empty() {
	return Err("Cannot split text into non-empty chunks".to_string());
	}
	return send_chunk_inner(
	chat_id,
	second,
	reply_to,
	message_thread_id,
	depth + 1,
	use_markdown,
	);
	}

[serrrfirat]

Looks good — well-structured recursive splitting with proper bounds. One medium finding posted inline.

[serrrfirat]

Medium Severity — Test passes for the wrong reason

test_find_split_midpoint_paragraph_boundary claims to test paragraph splitting, but the \n\n falls exactly at the mid_byte boundary (bytes 2000–2001), so text[..2001] only includes the first \n. rfind("\n\n") misses it and rfind('\n') matches instead — the test exercises newline splitting, not paragraph splitting.

This means a regression in the rfind("\n\n") branch of find_split_midpoint would go undetected.

Suggested fix: shift the \n\n earlier so it falls well within text[..mid_byte]:

let first = "A".repeat(1500);
let second = "B".repeat(2500);
let text = format!("{}\n\n{}", first, second);
let idx = find_split_midpoint(&text);
assert_eq!(idx, 1500); // rfind("\n\n") at byte 1500

[ilblackdragon]

HIGH — Adversarial boundary preference can exhaust MAX_SPLIT_DEPTH without meaningfully shrinking the payload.

find_split_midpoint searches backwards from the UTF-16 midpoint for \n\n -> \n -> ' ' and splits at the first match. When the early part of the chunk contains a paragraph/newline boundary but the late part is boundary-free (e.g. "Hello!\n\n" + "x".repeat(8000) — a rendered code block, URL, stack trace, hash, or base64 blob), the first boundary found near the start wins:

• Pass 1 (depth 0): split at byte 7. First half = "Hello!", second half = 8000 x's (~8000 UTF-16 units — still way over 4000).
• Pass 2 (depth 1): same problem on the 8000-x second half. No boundary, so hard-cut at mid ~ 4000 x's. Good.
• Pass 3 (depth 2): second half now ~4000 x's. Telegram may still reject for entity/emoji reasons. Depth limit hit -> Err(...) -> entire response dropped.

The natural-boundary preference is backwards-first-wins, which is fine when the boundary lives near the midpoint but pathological when it lives near the start. Consider: (a) requiring the boundary to be past some minimum offset (e.g. > mid_byte / 2), or (b) falling back to a hard cut at mid_byte when the chosen boundary would leave the second half still over-limit, or (c) raising MAX_SPLIT_DEPTH with an exponential-backoff style hard-cut floor. Real agent output (code blocks, JSON, SSH keys, base64) routinely has this front-loaded-boundary shape.

[ilblackdragon]

HIGH — No regression test covers the TooLong -> retry / split flow, only find_split_midpoint in isolation.

This is the exact 'test through the caller, not the helper' pattern in .claude/rules/testing.md: find_split_midpoint is a pure transform whose output gates a side effect (HTTP call to Telegram) through multiple layers (split_and_send -> send_chunk_inner). The PR adds five unit tests for the helper but zero tests for:

• send_chunk_inner receiving TooLong and splitting correctly
• Depth counter actually increments and terminates at MAX_SPLIT_DEPTH
• Markdown flag correctly flipping to false after a ParseEntities fallback AND propagating through subsequent split_and_send recursion
• Reply-thread ID chain: split_and_send returns the last sent id, not the first — a silent regression there would break threading on long messages

Since send_message calls channel_host::http_request directly (WIT binding), mocking is awkward, but the orchestration logic (depth, use_markdown plumbing, last-id semantics) could be extracted into a pure function that takes a Fn(&str, Option<&str>) -> Result<i64, SendError> and tested. Without this, the use_markdown flag bug the reviewer can't write (because it's hypothetical) would ship silently. The commit-msg regression-test hook likely passes because new tests were added, but they don't cover the actual bug surface the PR introduces.

[ilblackdragon]

MEDIUM — Final failure swallows the remaining chunks of the response.

When send_chunk_inner hits MAX_SPLIT_DEPTH and returns Err, the caller in send_response (around line 1437-1442) uses ? to propagate the error and abandons the loop. If chunk 2 of 5 fails to split, chunks 3-5 are never attempted — the user sees a partial reply and a silent error log. Given this PR exists because responses routinely exceed the length budget, the more defensive failure mode is: log the offending chunk, skip it with a placeholder (e.g. 'one segment was too long to deliver, see workspace log'), and continue sending subsequent chunks. Alternatively, a depth-exceeded fallback that hard-cuts at TELEGRAM_MAX_MESSAGE_LEN / 2 UTF-16 units (no boundary preference) would produce something deliverable.