Repository navigation
fix(channels): Telegram polling resilience and long-reply delivery - #123
Conversation
getUpdates used the API default of 100 updates. A backlog larger than the 256 KiB response buffer made curl abort before the offset advanced, so every later poll retried the same batch and the inbox stalled. Messages that omit from.id returned a hard error and pinned the same way. Request one update per poll, and skip a message with no sender id so the following update can be fetched. Supersedes #112.
Telegram treated a finished sendMessage as success, so an HTTP 400 for a reply over 4096 characters still acked cron and the user never saw the text. Discord refused the whole reply above 2000 characters, so the send failed and a due cron job re-ran the agent on every poll. Supersedes #117.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Context. ShellClaw is a C11 edge AI agent (Jetson/Pi) with pluggable channels (CLI, Telegram, Discord, WebChat) behind a channel_t vtable, cJSON for parsing, libcurl for HTTP, and one test binary per module. This PR touches only src/channels/* plus tests, and targets development.
Summary. The PR fixes three real problems: a Telegram update with no from.id pinned the poll offset, sendMessage failures were reported as success (curl OK, HTTP 400 ignored), and replies over the platform limit were dropped. The approach is sound and the Telegram side is well covered by regression tests. I could not build here (no libcurl dev package in the review VM), so I checked channel_outbound_chunk_len with a standalone harness instead of running make test. The main gaps are partial-delivery semantics when a later chunk fails, no Telegram 429 handling, and a behavior change for empty replies.
Must Fix
None found. Nothing here is a security issue or a broken contract.
Should Fix
- Partial delivery then retry duplicates output (
telegram.c:394-403,discord.c:960-972). If chunk 1 is sent and chunk 2 fails,tg_send/discord_sendreturn -1.send_then_maybe_ack_cron(src/core/dispatch.c:38-40) then skips the cron ack, so the job re-runs and chunk 1 is delivered again. Decide and document the contract (at-least-once is acceptable, but say so), or at minimum log which chunk index failed with the byte offset. - Telegram has no 429 handling (
telegram.c:286-307,310-335). Telegram limits a chat to roughly one message per second.RESPONSE_BUF_SIZEis 32 KiB, so a max-size reply is 8 back-to-backsendMessagecalls. A 429 is now correctly treated as non-delivery, but it aborts the rest of the reply and triggers point 1. Discord already retries on 429 withRetry-After. Readparameters.retry_afterfrom the error body and retry once or twice, or add a short delay between chunks. - Empty replies now fail on Telegram (
telegram.c:383). Before, an emptytextwent to the API, Telegram answered 400, curl still returned OK, andtg_sendreturned 0 so the cron job was acked. Now it returns -1.handle_messagecan produce an emptyresp_bufwitherr == 0(model returns only tool calls), so a cron-delivered job could re-run indefinitely. Discord already had this check, so it is consistent, but it is a behavior change that the PR description does not mention. Either treat empty as a no-op success indispatch.cor add a test that pins the intended behavior. - Missing tests for the send path. The chunk loops in
tg_sendanddiscord_sendhave no coverage, and that is where the new logic lives.channel_outbound_chunk_lenhas tests, but nothing checks that concatenating all pieces reproduces the input, nor a 4-byte code point at the boundary. Consider an injectable post function (or a test hook like the existing_for_testones) so the loop can be driven without a network. The AGENTS.md rule is regression test first, and the long-reply fix is the one without one. - The same chunk loop is copy-pasted in two channels. Move it into
channel_common.cas something likechannel_send_chunked(text, max_bytes, post_fn, ctx). That removes the duplicatedpiece[]buffer and bounds check, and gives item 4 a single place to test. It also helpsdiscord.c, which is now 1083 lines (already over the repo's 1000-line guideline) and grows again with this PR.
Nice to Have
channel_common.c:48: when the first character is wider thanmax_bytes, the function falls back tomax_bytesand splits the code point. I confirmedchannel_outbound_chunk_len("\xF0\x9F\x98\x80", 3)returns 3. It cannot happen with 2000/4096 caps, but the header says it never splits a code point. Either document the fallback inchannel.hor add a test that pins it.- Limits are in bytes, but Telegram (4096) and Discord (2000) count characters. Conservative and safe, but CJK or emoji-heavy replies get split about 3x earlier than needed. Worth a comment so nobody "fixes" it later.
- Splits land mid-word and can cut a Markdown code fence. Preferring the last newline or space inside the window would read better.
telegram_post_chunkcreates a new curl handle (new TLS handshake) per chunk. Reusing one handle across the loop is cheap to do.discord.c:936: the trailingfree(json_body)after the retry loop is unreachable, since every loop exit path already frees it.tests/test_telegram.c:268:MU_RUN(test_getupdates_url_requests_one_update);is not indented like its neighbours.- Several new comments describe the old bug ("used to fail", "left last_update_id unchanged"). That history belongs in the commit message. Keep the comment to the why: "skip so the offset advances".
- The
fromskip test covers a missingfrom. Add cases forfromas a non-object andidas a string, since the new condition handles both. - Remaining
return -1paths inparse_updateafter a validupdate_id(malformedfirst, missingupdate_id) can still wedge the offset. Unlikely from Telegram, but worth a follow-up.
Positive Highlights
limit=1is a good root-cause fix. The parser only ever readresult[0], so each poll was re-downloading and discarding the rest of the batch, and a large backlog could exceed the 256 KiB buffer and never advance. The URL builder is also unit tested with bounds checks.telegram_response_okchecks both HTTP status andok:true, andtelegram_perform_postnow installs a write callback, so the API response no longer leaks to stdout. The tests cover 400, 429,ok:falseand empty body.- Discord's old behavior of failing the entire send on >2000 bytes is replaced by chunking, and the
discord_send/discord_post_textsplit keeps the retry logic intact and each function short. The shared UTF-8-aware helper is small, documented with an example, and tested at the multibyte boundary.
Sent by Cursor Automation: Code review
|
|
||
| (void)attachments; | ||
| (void)att_count; | ||
| if (!g_tg.initialized || !g_tg.token || !recipient || !text || text[0] == '\0') |
There was a problem hiding this comment.
Behavior change: empty text used to reach the API (400, but curl OK, so 0 was returned and cron was acked). Now it returns -1, which skips the cron ack in dispatch.c and can re-run the job. handle_message can pass an empty resp_buf with err == 0. Please decide the intended behavior and pin it with a test.
| return -1; | ||
| cursor = text; | ||
| while (cursor[0] != '\0') { | ||
| char piece[TG_TEXT_MAX + 1]; |
There was a problem hiding this comment.
If a later chunk fails (429, network), earlier chunks are already delivered and the caller sees -1, so cron re-runs and resends them. At minimum log the failing chunk offset. Telegram also rate limits per chat (~1 msg/s), and a 32 KiB reply is up to 8 sends back to back with no retry on 429.
| n = max_bytes; | ||
| while (n > 0 && ((unsigned char)text[n] & 0xC0) == 0x80) | ||
| n--; | ||
| return n > 0 ? n : max_bytes; |
There was a problem hiding this comment.
When the leading character is wider than max_bytes this returns max_bytes and splits the code point (chunk_len("\xF0\x9F\x98\x80", 3) returns 3). Unreachable with the current caps, but it contradicts the header contract. Document it or cover it with a test.
| * the whole send, so cron kept re-running the agent and the user saw nothing. */ | ||
| cursor = text; | ||
| while (cursor[0] != '\0') { | ||
| char piece[DISCORD_MSG_MAX_LEN + 1]; |
There was a problem hiding this comment.
This loop is identical to the one in tg_send. Consider a shared channel_send_chunked() in channel_common.c so it is written and tested once. discord.c is already past 1000 lines.
| n = channel_outbound_chunk_len(euro, 2); | ||
| MU_ASSERT(n == 1, "multibyte character is not split"); | ||
| MU_ASSERT(channel_outbound_chunk_len(euro + n, 3) == 3, "next piece is the whole euro sign"); | ||
| } |
There was a problem hiding this comment.
Good start. Please add a round-trip check (walk a long mixed ASCII/multibyte string with a small cap and assert the pieces concatenate to the original) and a 4-byte code point at the boundary.
adriannoes
changed the title
tg_send and discord_send carried a copy-pasted piece[] loop (bounded copy, rune-safe cut, POST, advance). Move it once into channel_common.c as channel_send_chunked(text, max_bytes, post, ctx, fail) with the NUL-terminated piece buffer, so bounds-checking is a single concern and the piece copy lives in one place. Both channels now post through a small callback; this also shrinks discord.c, which is past the 1000-line guideline. channel_chunk_fail_t reports the failing chunk index and byte offset so a caller can log partial delivery, and the header documents two_limits contract notes: the chunk_len fallback when the first rune is wider than max_bytes, and that limits are BYTES even where Telegram counts characters (conservative so nobody 'fixes' it to code points later). Cover the helper network-free through the post callback: concatenated pieces reproduce the input exactly (multibyte, 4-byte cap), single-piece fast path, a callback failing on chunk 2 reports index 2 / offset 8, and invalid-argument and empty-text contracts. Extend the chunk_len tests with a 4-byte code point at the split edge. Refs: #123
When a chunk POST fails mid-reply the send returns non-zero and dispatch skips the cron ack, so the job re-runs and any already -delivered chunks are delivered again (at-least-once). Log the failing chunk index and its byte offset in both channels so the duplicate boundary in a re-run is traceable from the daemon log. Refs: #123
Telegram allows about one message per second per chat; a 32 KiB cron or chat reply is up to 8 back-to-back sendMessage calls at the 4096 cap, and a 429 aborts the rest of the reply (which then re-runs, see partial-delivery logging). Pause 500ms between consecutive chunk POSTs — never before the first — to stay under the chat rate. Full 429 handling (parameters.retry_after parse + wait + retry) is a deferred follow-up; this is the deliberate lighter alternative. Refs: #123
The three fixes carried by PR #123 are user-visible delivery fixes: getUpdates batch resilience (a bad batch no longer pins the offset and kills polling), sendMessage delivery actually verified (HTTP 400 is not success), and long replies chunked for Telegram/Discord within platform limits. Record them under [Unreleased] Fixed, matching the existing bullet style. Refs: #123
|
Review response — done in 2d87216 (both channels now share one chunked-send helper in channel_common, with round-trip, multibyte-boundary and failing-chunk tests), 82ea234 (failing chunk index and byte offset are logged on partial sends), 14fffcf (500 ms pacing between Telegram chunks), and 592b5ce (changelog). Deferred: the empty-reply policy (belongs to send_then_maybe_ack_cron in dispatch.c — handled with the gateway group that touches dispatch), full 429 retry_after handling, and the smaller nice-to-haves. |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # Makefile
1 participant
Summary
Consolidates two cursor high-severity channel fixes as operator-authored commits:
Consolidation note: the overlapping test regions in tests/test_telegram.c were merged so both regression suites run (url-building single-update test and response_ok test).
Testing
make test_telegram— 47 tests, 0 failedmake test_channel— 18 tests, 0 failedCI=true make test— passedmake static— passedSupersedes #112 and #117.