fix: Telegram double delivery, dead-bot crash, 429s, out-of-order chunks - #76
Merged
Conversation
The Telegram frontend double-delivered every streamed assistant/thinking block: the final session.message rebroadcast sent the full content but never dropped the delta buffer, so the idle flush re-sent it. Worse, any unrelated session.message (e.g. a tool_call mid-stream) flush-deleted the LIVE buffer, dropping all later deltas and duplicating the flushed prefix when the final broadcast arrived. Long messages also chunked in parallel (out-of-order >4000-char output, "Done." overtaking content), and thinking flushes died silently on Telegram markdown parse errors. Extract the buffering into StreamRelay (stream.ts): - final full broadcast is authoritative: send the unflushed tail, drop the buffer - interleaving messages partial-flush live buffers via a per-buffer flushed offset instead of deleting them — later deltas keep appending - all sends go through one promise chain: chunks sequential, line-boundary splits, "Done." queued after the flush - thinking sends retry as plain text when Markdown parsing fails Tests exercise the real StreamRelay through the RelayApi seam. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, honor 429s
Any thrown handler error permanently stopped long polling — grammy's
default error handler stops the bot and re-throws. A deterministic trigger
existed in /ls, which rendered session status and workdir unescaped in
MarkdownV2: a tool_running status underscore is a Telegram 400, which
killed the bot for good.
- install bot.catch to log and keep processing updates
- escape /ls status with escMd and workdir with escCode (inside a
MarkdownV2 code span only backtick and backslash are special; escMd
there would render literal backslashes)
- install @grammyjs/auto-retry as an API transformer so 429s wait for
retry_after and retry instead of being swallowed by .catch(() => {})
- consolidate the duplicate esc/escMd helpers into stream.ts
Tests drive a real grammy Bot with a stubbed API transformer through
handleUpdates (the polling entry point): a 400 reply no longer stops
update processing, and a flood-limited send is retried once.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ch/switch Tool completed/cancelled states never rendered in Telegram: the daemon broadcasts them as session.message.delta with toolStateUpdate, but the delta handler only read contentAppend — tools showed ⚡ forever. And session switch/detach cleared the streaming buffers outright, silently discarding any streamed-but-undelivered content. - remember messageId → tool name from the tool_call broadcast and render ✓ name / ✗ name failed / ✗ name cancelled when a toolStateUpdate delta arrives, flushing streamed text first so lines land in order - replace clear() with flushAndClear() on session switch and /detach: buffered content is delivered with a "✂️ detached mid-stream" marker instead of vanishing - update the session-switch mirror tests to the flush-before-clear contract Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces ChangesTelegram StreamRelay Refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TelegramFrontend
participant StreamRelay
participant RelayApi
TelegramFrontend->>StreamRelay: handleDelta(chatId, delta)
StreamRelay-->>StreamRelay: buffer text by messageId
TelegramFrontend->>StreamRelay: handleMessage(chatId, message)
StreamRelay-->>StreamRelay: flush unrelated buffer tail
TelegramFrontend->>StreamRelay: flushIdle(chatId)
StreamRelay->>RelayApi: sendMessage(chatId, bufferedText)
StreamRelay->>RelayApi: sendMessage(chatId, "✅ Done.")
sequenceDiagram
participant User
participant TelegramFrontend
participant StreamRelay
User->>TelegramFrontend: /attach or /detach
TelegramFrontend->>StreamRelay: flushAndClear(chatId)
StreamRelay->>StreamRelay: deliver buffered content
StreamRelay->>StreamRelay: clear buffers
TelegramFrontend-->>User: escMd-escaped confirmation reply
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #76 +/- ##
==========================================
- Coverage 80.72% 79.93% -0.79%
==========================================
Files 56 59 +3
Lines 7895 8890 +995
==========================================
+ Hits 6373 7106 +733
- Misses 1522 1784 +262
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tests/telegram-bot.test.ts (1)
54-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo teardown stops the parked polling loop.
fe.start()kicks off long polling against this stub, andgetUpdatesis parked on a promise that only settles ifsignalaborts (i.e., only onbot.stop()). Neither test callsbot.stop()/fe.stop()afterward, so each test leaves a dangling never-resolving promise plus an active polling loop. Bun's test runner will likely force-exit the process, but this accumulates open handles across the suite and risks hangs if run under different configurations (e.g.,--watch, leak detection).♻️ Suggested cleanup
- const fe = new TelegramFrontend("42:TEST_TOKEN", [ALLOWED_USER], bot); - await fe.start(fakeContext()); + const fe = new TelegramFrontend("42:TEST_TOKEN", [ALLOWED_USER], bot); + await fe.start(fakeContext()); + // ... test body ... + await bot.stop().catch(() => {});Alternatively wrap with
try { ... } finally { await bot.stop().catch(() => {}); }in eachit, or add anafterEachthat tracks and stops the bot created per test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/telegram-bot.test.ts` around lines 54 - 71, The stubbed polling loop in makeStubbedBot is left running because fe.start() is never paired with a stop, so each test leaks an active getUpdates promise. Update the telegram-bot tests to ensure every Bot created by makeStubbedBot is torn down after each test, ideally by calling bot.stop() or fe.stop() in a finally block or shared afterEach cleanup, so the abort listener on getUpdates is triggered and no polling handle remains open.src/tests/telegram-session-switch.test.ts (1)
120-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock flush semantics are simplified relative to
StreamRelay.flushAndClear.
makeDeps().flushAndClearonly pushes non-emptybuf.contentand clears the map; it doesn't model the real implementation's partial-flush tracking (buf.content.length > buf.flushed) or emit the "✂️ Detached mid-stream" marker. This is fine as a higher-level mirror (the real flush semantics are covered intelegram-stream.test.ts), but worth keeping in mind if this mock and the productionStreamRelaydrift further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/telegram-session-switch.test.ts` around lines 120 - 142, The `makeDeps().flushAndClear` stub in `makeDeps` is too simplified compared with `StreamRelay.flushAndClear`, so update it to mirror the production behavior more closely by only flushing newly produced content based on the buffer’s flushed state and by handling the detached mid-stream marker consistently. Keep the helper aligned with the real `flushAndClear` semantics used by `StreamRelay` so this test stays representative if the implementation changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontends/telegram/index.ts`:
- Around line 349-351: The /attach and /detach flow in the Telegram frontend is
confirming the session switch before the buffered relay messages finish sending.
In `src/frontends/telegram/index.ts`, after calling
`state.relay.flushAndClear(chatId)` in the attach/detach handling path, await
`settle()` on the relay before sending the user-visible confirmation so the old
buffered output and detached marker are delivered first. Use the existing
`flushAndClear` and `settle` relay methods in the switch/detach handler to keep
message order correct.
- Around line 739-742: `#forwardToChat` currently forwards any message as long
as user state exists, so stale daemon callbacks from a previous session can
still reach the active relay after detach or session switch. Update
`#forwardToChat` to validate `msg.sessionId` against the current session
information in `state` (including `attachedSessionId`/active session identity)
before accessing `state.relay`, and return early when the session does not
match. Keep the check near `state.relay` in `src/frontends/telegram/index.ts` so
old-session messages are dropped before they are forwarded.
---
Nitpick comments:
In `@src/tests/telegram-bot.test.ts`:
- Around line 54-71: The stubbed polling loop in makeStubbedBot is left running
because fe.start() is never paired with a stop, so each test leaks an active
getUpdates promise. Update the telegram-bot tests to ensure every Bot created by
makeStubbedBot is torn down after each test, ideally by calling bot.stop() or
fe.stop() in a finally block or shared afterEach cleanup, so the abort listener
on getUpdates is triggered and no polling handle remains open.
In `@src/tests/telegram-session-switch.test.ts`:
- Around line 120-142: The `makeDeps().flushAndClear` stub in `makeDeps` is too
simplified compared with `StreamRelay.flushAndClear`, so update it to mirror the
production behavior more closely by only flushing newly produced content based
on the buffer’s flushed state and by handling the detached mid-stream marker
consistently. Keep the helper aligned with the real `flushAndClear` semantics
used by `StreamRelay` so this test stays representative if the implementation
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a51b7a75-2ffa-484f-880c-d379f4ac3caf
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!**/*.lock,!bun.lock
📒 Files selected for processing (6)
package.jsonsrc/frontends/telegram/index.tssrc/frontends/telegram/stream.tssrc/tests/telegram-bot.test.tssrc/tests/telegram-session-switch.test.tssrc/tests/telegram-stream.test.ts
…tale session broadcasts Address CodeRabbit review on PR #76: - flushAndClear only queues sends on the relay chain, so the "Attached to …" / "Detached from …" confirmations (sent outside the relay) could overtake the flushed buffer tail and interruption marker. Await relay.settle() after flushing in both handlers. - #forwardToChat forwarded any broadcast as long as user state existed; an in-flight callback from the old session arriving after a detach or switch reached the current relay/chat — a stale idle status would even flush the new session's buffers and print a bogus "✅ Done.". Gate session-scoped messages on msg.sessionId === attachedSessionId via the exported isStaleBroadcast helper (messages without a sessionId are never dropped). - test hygiene (review nitpick): stop the stubbed bot in a finally block so no parked getUpdates polling handle leaks; the stub answers grammy stop()'s signal-less offset-save getUpdates immediately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Addressed the review in 45f0ca5:
695 pass / 0 fail, biome + tsc clean. |
Collaborator
Author
|
Review addressed through 45f0ca5:
Suite: 695 pass / 0 fail, biome + tsc clean. |
codecov/patch failed at 76.82% — the PR's handler wiring in src/frontends/telegram/index.ts (attach/switch/detach, forwardToChat, /ls//new//destroy//search) was untested because every path sits behind /auth. Add telegram-flows.test.ts: a local Bun.serve JWKS endpoint plus an ES256-signed JWT drives the REAL verifyToken/ZeroID verification, and a recording fake SessionManager captures the AttachedClient so daemon broadcasts exercise the real forwarding path via the injectable-bot seam. Covers: authenticated /ls escaping, /new, /destroy, oversized /search plain-text fallback, attach + streamed turn + stop button + idle Done, stale-session gating, approval prompt + Approve tap, switch/detach flush-before-confirm ordering, and failed re-attach restore. Local patch coverage for index.ts changed lines: 50/51 (98%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
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
1 participant
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.
Fixes (from the rendering/session-scaling audit)
P0 — every streamed assistant/thinking block delivered twice: the daemon's final full-message broadcast was sent immediately, but the delta buffer for that messageId was never dropped, so the idle flush re-sent the same content on every normal turn. The new
StreamRelaytreats the final broadcast as authoritative: it sends only the unflushed tail and deletes the buffer.P0 — one thrown handler error permanently killed the bot: grammY's default error handler calls
stop()and rethrows — long polling never resumes. Deterministic trigger:/lsrendered session status and workdir unescaped in MarkdownV2 (atool_runningunderscore → Telegram 400 → crash). Now:bot.catchlogs and keeps polling; status escaped withescMd, workdir with a newescCode(it sits in a code span, where only backtick/backslash are special — plainescMdwould render literal backslashes).P1 — no 429 handling:
@grammyjs/auto-retrytransformer honorsretry_afterinstead of the old fire-and-forget.catch(() => {})silently dropping flood-limited messages.P1 — mid-stream buffer kill: any interleaving broadcast (e.g. a parallel subagent's tool_call) used to flush-and-delete the live buffer — later deltas were dropped and the prefix duplicated. Replaced with per-buffer flushed offsets: interleavings flush the unflushed tail, the buffer stays live, delivery is exactly-once and in-order.
P1 — thinking blocks silently dropped:
parse_mode: Markdownon raw model reasoning 400s on any unbalanced*/_/backtick; now retries as plain text.P2 — out-of-order chunks: all sends flow through one promise chain — chunks of >4000-char messages go sequentially, split on line boundaries, and "✅ Done." can no longer overtake the content it announces.
P2 — tool ✓/✗ never rendered: completion/cancellation arrives as a
toolStateUpdatedelta the old handler ignored; the relay remembers tool names from the tool_call broadcast and renders terminal states.P2 — buffers discarded on switch/detach: buffered undelivered content now flushes with a "✂️ Detached mid-stream" marker instead of vanishing.
Design
Streaming logic extracted from
index.tsintostream.ts(StreamRelaybehind a minimalRelayApiseam), so the 30 new tests drive the real production code with a fake Telegram API instead of mirroring the logic. The bot.catch test drives a real grammYBotthroughhandleUpdatesand hangs without the fix.Tests
30 new across
telegram-stream.test.ts,telegram-bot.test.ts, and updatedtelegram-session-switch.test.ts: exactly-once delivery (4 variants), thinking exactly-once, interleaved tool_call no-dup/no-drop/in-order, /ls escaping, sequential chunking + Done-marker ordering, toolStateUpdate rendering (✓/✗/cancelled), auto-retry on 429, flush-on-detach markers.Full suite: 687 pass / 0 fail. biome + tsc clean.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes