fix(telegram): bound and latency-gate ◦ tool-progress lines#702
fix(telegram): bound and latency-gate ◦ tool-progress lines#702griffinwork40 wants to merge 1 commit into
Conversation
The Telegram live preview appended one `◦` progress line per tool round to `accumulated` with no bound and no delay, so every turn — including fast ones — churned the message and a tool-heavy turn grew it without limit. `cleanFinal` already deleted the noisy preview at the end, so scrollback was clean; the problem was purely the live experience, and there was no knob to tune it. Four changes, all inside `streamResponse`: - Latency gate: progress lines stay hidden until the turn has run PROGRESS_START_DELAY_MS (5s). Withheld lines are still RECORDED, so when the gate opens the region shows the recent rounds — a render delay, not a drop. Most turns now go straight from `Thinking…` to the answer with zero `◦` churn. - Rolling window: the region is a bounded, rewritable tail of `accumulated` (MAX_PROGRESS_LINES = 6) instead of an append-only log — the same treatment `renderSubagentFooter` already applies to sub-agent steps. - Suggestion dedupe now compares against `answerText` rather than `accumulated`. The gate means "don't echo text already rendered as the answer", but `accumulated` also carries the `◦` region, which made the comparison spuriously unequal and appended a duplicate 💡 to the answer on exactly the tool-heavy turns that produce progress. - Activity receipt: the clean final message gains a one-line `⏱️ N steps · Ns` summary, so the turn's cost survives the preview deletion. Appended at DELIVERY only — never to `answerText`, which `onComplete` records into resumable session history. Two couplings worth calling out, both found by adversarial re-derivation and each now covered by a regression test that fails if the coupling is broken: - The progress handler's `inContentRun = false` is NOT cosmetic — it is the stream_retry round boundary. Gating it off (the obvious way to implement "hide progress") leaves contentRunStart* at an older offset, so a later stream_retry truncates MORE of `answerText` than the retry produced, silently destroying already-streamed answer text. The render is gated; that assignment stays unconditional. - With progress withheld, a progress-only turn (reachable via an abort mid tool-loop, which yields turn.completed with no assistant text) left `accumulated` empty and stranded the user on the bare `Thinking…` placeholder. The `done` branch now flushes the withheld region explicitly. `progressDelayMs` is exposed on the options object so tests assert rendering without depending on wall-clock timing. Operator-facing configuration for the delay/window is deliberately left to a follow-up. Tests: 8 new cases (latency gate, rolling window, retry-boundary regression, dead-turn guard, receipt present/absent/not-recorded, suggestion dedupe, pure render helpers). Full suite green: 13091 passed. Signed-off-by: Griffin Long <griffinwork40@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f56983a61e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event.type === 'chunk' && event.chunk.type === 'content') { | ||
| // Answer text follows the progress region, so the region is now frozen | ||
| // history — stop rewriting it (see the progressRunStart invariant). | ||
| endProgressRun(); |
There was a problem hiding this comment.
Keep progress bounded across content chunks
When a provider alternates content with tool rounds, as the Anthropic and OpenAI-compatible loops can, endProgressRun() clears the rolling state but leaves the already-rendered progress region frozen in accumulated. Each later progress event therefore starts another region instead of replacing the previous one; an alternating nine-round stream retains all nine ◦ lines rather than six. Tool-heavy previews consequently remain unbounded and can cross Telegram's message limit, after which sendOrEdit edits only the first chunk and recent progress disappears. Keep a single replaceable progress region independent of interleaved answer content.
Useful? React with 👍 / 👎.
| if (progressLines.length > MAX_PROGRESS_LINES) { | ||
| progressLines = progressLines.slice(-MAX_PROGRESS_LINES); | ||
| } | ||
| if (Date.now() - turnStartedAt >= progressDelayMs) { |
There was a problem hiding this comment.
Open the progress gate when the delay expires
The latency gate is evaluated only when another progress event arrives. If the first round finishes before five seconds and the next tool then runs silently for a long time, nothing schedules a render when the threshold expires; when the next content chunk arrives, endProgressRun() discards the withheld line, so the user remains on Thinking… for the entire long operation and the recorded progress is never shown. Schedule a delayed refresh when the first line is withheld, with cancellation at completion, rather than relying on a later progress event.
Useful? React with 👍 / 👎.
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Merge decision: ✅ MERGE
Reviewed against branch HEAD f56983a6. Regime: full (327 lines, 2 files). Dimensions: security, api-compat, correctness, spec-compliance, test-coverage, perf-observability.
No critical, high, or medium findings. The four claimed invariants were each independently re-derived at the reviewed ref, not taken on the PR description's word.
Invariant verification (the load-bearing checks)
1. accumulated writer audit — PASS. Every mutation site enumerated at f56983a6 and checked against the progressRunStart offset invariant documented at streaming.ts:290-297:
| Line | Writer | endProgressRun() |
|---|---|---|
| 575 | content chunk | precedes (569) ✅ |
| 584 | stream_retry rollback |
follows (589) — safe, see nit |
| 596 | assistant message | follows (601) — safe, see nit |
| 641 | progress rewrite | n/a — this is the mechanism |
| 660 | suggestion | precedes (659) ✅ |
| 758 | done-branch flush |
post-loop; no further progress events |
No writer is missed. The two "follows" sites are still correct: there is no await between the mutation and the later endProgressRun() (584→589, 596→601), so the event loop cannot interleave a progress event and reach the rewrite at 641 with a stale offset.
2. inContentRun = false stays unconditional — PASS. streaming.ts:617 is the first statement in the progress branch (608), outside the progressDelayMs gate at 638. The render is gated; the stream_retry round boundary is not. This is the coupling the PR body flags, and the implementation matches the claim.
3. Progress-only turn is not stranded — PASS. streaming.ts:749-759 flushes the withheld region when answerText is empty and accumulated is blank. Recording at 633-637 is never gated, so progressLines survives a fully-withheld turn.
4. Receipt kept out of resumable history — PASS. streaming.ts:738-740 appends renderActivityReceipt(...) only as the argument to deliverClean; answerText is never reassigned, and onComplete(answerText, …) at 765 passes the unmodified value.
Dimension results
- security — no issues found (read
streaming.ts,formatter.tsatf56983a6). No new data reaches Telegram: the◦line interpolates the same threeevent.progressfields through the same unchangedsanitizeLabel()calls as onmain; only control flow changed.renderActivityReceipt(164-187) interpolates two integers. No injection vector —markdownToTelegramHtmlescapes&/</>unconditionally before building its own tag set (formatter.ts:90-93), andformatter.tsis untouched by this diff. - api-compat — no issues found.
renderProgressRegion/renderActivityReceiptare additive exports with no prior consumers.progressDelayMs?: numberis a new optional field on an already-optional options bag; the sole production callersrc/telegram/handlers/message.ts:865does not set it and inherits the 5s default. No required parameter changed. - correctness — no issues found beyond the nit below.
- spec-compliance — assessed against the PR title + body. No unmet intent; all four described changes are present and behave as described. No scope creep: the diff touches only
streaming.ts/streaming.test.ts, and each item the body declares out of scope (operator config /ENV_REGISTRY,<blockquote expandable>, the pre-existing empty-answerTextonCompletebug) is in fact left untouched. - test-coverage — the "no assertion was weakened" claim holds. The diff contains zero removed
expect(lines; both pre-existing tests changed only by appending, undefined, { progressDelayMs: 0 }to the call plus explanatory comments — every assertion line is unchanged context. - perf-observability — growth is bounded twice over:
progressLinesis trimmed toMAX_PROGRESS_LINESon every push (634-636) andrenderProgressRegionre-slices to the same bound (165). The per-round rewrite slicesaccumulatedinstead of appending, so the region keeps fixed height across a long tool-heavy turn.
Findings
nit · high confidence · docs-vs-code · src/telegram/streaming.ts:295-297 · ref:f56983a6 · citation-type: file-state
finding: The invariant comment states every other writer must call endProgressRun() "first", but the stream_retry (584) and assistant-message (596) writers call it after mutating accumulated, so the comment does not describe the code at those two sites.
evidence:
// appends to `accumulated`, so EVERY other writer (content chunk, assistant
// message, suggestion, stream_retry rollback) must call `endProgressRun()`
// first — otherwise the rewrite would truncate their text.
suggestion: Reword to "…must call endProgressRun() before the next progress render" (which is what actually holds, and is what makes 584/596 safe), or reorder those two sites to match the content-chunk/suggestion pattern.
CI note
Test (windows-latest) is red, but not because of this PR. The failures are in tests/agent/memory/memory-loader.test.ts, which this diff does not touch, and the same job fails identically on main (run 30137517004, HEAD 62df42c5). The matrix is continue-on-error for non-ubuntu by design (.github/workflows/ci.yml:83-94) — ubuntu is the required gate and is green. Not a merge blocker for #702.
What was not checked
- Read against branch HEAD
f56983a6; all citations verified at that ref. Nocritical/highfindings, so the citation-verification and/shadow-verifywaves were not triggered. - Stated intent: PR #702 title + body — spec-compliance was assessed.
- Tests were read statically, not executed; the PR's own reported run (13091 passed) was not independently reproduced.
- Live Telegram delivery was not exercised (no integration harness), so the 5s gate's real-world feel is unverified — the PR notes this too.
- Per-line byte length of a progress line is not bounded by this change (only line count is). Not a regression — the prior append-only path had no length bound either — and
splitLongMessagehandles the 4096 ceiling downstream. Flagged as out-of-scope context, not a finding.
griffinwork40
left a comment
There was a problem hiding this comment.
🤖 Automated review (hourly sweep), generated by the /review tool — a maintainer will follow up.
Code Review — PR #702
fix(telegram): bound and latency-gate ◦ tool-progress lines
Reviewed ref: f56983a6 · base main · 327 lines across 2 files · regime: full (2 review dimensions + adversarial re-derivation)
Merge decision: MERGE — with one medium follow-up noted below
No critical, high, or blocking findings. The load-bearing invariant this PR rests on was independently verified, not taken on the description's word.
Findings
1. medium · confidence: high · correctness
A progress-only turn that exits without a terminal event strands the user on the bare Thinking… placeholder — the exact failure the PR's own "dead-turn guard" fixes, but only on the done path.
src/telegram/streaming.ts:808-820 · ref: f56983a6 · citation-type: file-state
const preview = sentMessage as Message.TextMessage | null;
if (preview && !sawTerminalEvent) {
const full = cleanFinal && answerText.trim() ? answerText : accumulated;
if (full.trim()) {The done branch got an explicit withheld-region flush (streaming.ts:749-759, else if (progressLines.length > 0)). The non-terminal post-loop branch did not. With the gate closed, accumulated stays empty (streaming.ts:641 is the only writer and it is inside if (Date.now() - turnStartedAt >= progressDelayMs)), so full.trim() is false and nothing is delivered; the else if at :820 also requires a truthy accumulated and is skipped too. progressLines is referenced nowhere after :758.
This is a regression introduced by this PR, not a pre-existing gap. At base 09a3477e the progress handler appended unconditionally (streaming.ts:536-537, accumulated += line), so accumulated was non-empty on this path and the block delivered it.
Trigger conjunction (narrow — why this is medium, not high): progress-only turn · every progress event lands inside progressDelayMs (5s default) · zero content/message/suggestion events · the provider stream ends by graceful iterator close with no done/error. A watchdog timeout (:478) or error event (:775) throws past this block instead, so those paths are unaffected.
Suggestion: mirror the :749 guard — add else if (progressLines.length > 0) to the :808 branch, feeding renderProgressRegion(progressLines) into full.
2. nit · confidence: high · correctness
On the stream_retry and assistant-message writers, endProgressRun() runs after the buffer mutation (:584→:589, :596→:601) rather than before, unlike the content and suggestion writers. Harmless today — nothing reads progressRunStart in between — but it diverges from the "every other writer calls endProgressRun() first" wording documented at :290-297.
Verified — the invariant this PR rests on
Every writer to accumulated clears the progress run in the same handler. Enumerated at the reviewed ref:
| Writer | Site | Clears run? |
|---|---|---|
| content chunk | :575 |
✅ endProgressRun() at :569 (before) |
| stream_retry rollback | :584 |
✅ at :589 (after mutation, same handler) |
| assistant message | :596 |
✅ at :601 (after mutation, same handler) |
| progress region rewrite | :641 |
n/a — is the progress writer |
| suggestion | :660 |
✅ at :659 (before) |
| done-branch flush | :758 |
n/a — terminal |
inContentRun = false is unconditional at streaming.ts:617 — outside the render gate at :640, as the PR claims. The retry-boundary coupling is intact.
Verified — test claims
Checked against the actual test file, not the description:
- Retry-boundary regression asserts the full string:
expect(recorded).toBe('AAACCC')(streaming.test.ts:1016) — it fails if the coupling breaks. - Latency-gate test uses the production default — no
progressDelayMsoverride (:964, comment at:954). - Dead-turn guard present (
:1020), asserts the placeholder is not the last thing sent (:1038). - Receipt purity asserted:
expect(recorded).not.toContain('⏱️')(:1064). - Rolling window boundary asserted at
:975-979(9 rounds, window 6 → keeps 4..9, drops 1..3). - "No assertion was weakened" holds mechanically: the test file deletes exactly 3 lines — one import and two call sites — and the diff removes zero
expect/assertion lines anywhere.
Verified — security · api-compat
- No secret/token exposure:
renderActivityReceipttakes only numeric counters (:184-188). - No injection:
renderProgressRegion/renderActivityReceiptare pure string builders; all sends still route throughmarkdownToTelegramHtml, andsanitizeLabelstill runs on progress fields (:624-626) — unchanged by this diff. - No API break:
progressDelayMs?: number(:226) is a new optional field. Sole production caller issrc/telegram/handlers/message.ts:865, which does not set it and picks up the 5s default — a behavior change the PR discloses explicitly. - Receipt never reaches persisted history:
:738-739concatenates for delivery only;:765passes the un-concatenatedanswerTexttoonComplete.
Spec-compliance vs. stated intent
All four numbered changes MET (gate :307+:640; window :635-636; dedupe vs answerText :658; receipt :738). Both declared couplings MET and genuinely test-guarded. No scope creep found. Declared out-of-scope items (operator config, <blockquote expandable>, the pre-existing empty-answerText persistence bug) were treated as out of scope, not as defects.
CI note
Test (windows-latest) is red, but this is pre-existing and not caused by this PR — the same job fails on main (runs 30137517004, 30128726777, 30127658645). Failures are path-separator assertions across ~40 unrelated files (e.g. expected [ '/base', 'D:\extra\read' ] to include '/extra/read'). src/telegram/streaming.test.ts reports zero failures on Windows. Every other check is green.
What was not checked
- Read against branch HEAD
f56983a6; all citations verified at that ref, and the base comparison against09a3477e. - Live Telegram delivery — no integration harness (the PR discloses this).
- The tests were not executed locally; CI results on ubuntu/macOS were taken as the signal.
- Stated intent: PR #702 title + body — spec-compliance assessed.
Summary
The Telegram live preview appended one
◦progress line per tool round toaccumulatedwith no bound and no delay. Every turn — including fast ones — churned the message, and a tool-heavy turn grew it without limit.cleanFinalalready deletes the noisy preview at the end, so scrollback was clean; the problem was purely the live experience, and there was no knob to tune it.Prior art check: OpenClaw (the closest analogue — self-hosted agent over Telegram) uses the same send-and-edit transport we do, and solves this with a bounded rolling window (
maxLinesdefault 8) plus a 5s start delay. This lands the same shape. Notably it does not need Telegram's newersendMessageDraft(Bot API 9.3+), which ourtelegraf@4.16.3(Bot API 7.1) couldn't call anyway.Four changes, all inside
streamResponse:PROGRESS_START_DELAY_MS= 5s)◦lines — straight fromThinking…to the answer. Withheld lines are still recorded, so the gate opening shows recent rounds: a render delay, not a drop.MAX_PROGRESS_LINES= 6)accumulatedinstead of an append-only log — the same treatmentrenderSubagentFooteralready applies to sub-agent steps.answerText, notaccumulatedaccumulatedalso carries the◦region, making the comparison spuriously unequal and appending a duplicate 💡 on exactly the tool-heavy turns that produce progress.⏱️ N steps · Ns)answerText, whichonCompleterecords into resumable session history.Two couplings worth reviewer attention
Both were found by adversarial re-derivation before implementation, and each now has a regression test that fails if the coupling is broken (verified by deliberately reintroducing the bug):
inContentRun = falsein the progress handler is not cosmetic — it is thestream_retryround boundary. Gating it off (the obvious way to implement "hide progress") leavescontentRunStart*at an older offset, so a laterstream_retrytruncates more ofanswerTextthan the retry produced — silently destroying already-streamed answer text. The render is gated; that assignment stays unconditional. Guard:REGRESSION: a gated-off progress round still sets the stream_retry boundary…(assertsAAACCC, notCCC).turn.completedwith no assistant message),accumulatedwas empty and the user was stranded on the bareThinking…placeholder. Thedonebranch now flushes the withheld region explicitly. Guard:dead-turn guard: a fast progress-only turn still delivers….The
progressRunStartoffset is only valid while nothing else appends toaccumulated, so every other writer (content chunk, assistant message, suggestion,stream_retryrollback) callsendProgressRun()first. That invariant is documented at the declaration.Deliberately out of scope
progressDelayMsis exposed on the options object as a test seam only; atelegram.progressconfig key would pull inENV_REGISTRY+docs/env-registry.*sync and is better as a follow-up.<blockquote expandable>for collapsed detail. Investigated and rejected for now:markdownToTelegramHtmlescapes</>unconditionally before injecting its own fixed tag set (formatter.ts:90-93), so an injected tag renders as literal text. Would need the tag emitted inside the formatter plus tag-awaresplitLongMessage.onCompletefires withanswerText === ''and persists{assistant: ''}into resumable history with no non-empty check (streaming.ts→session-stats.ts). Not caused by this change and not fixed here; worth its own issue.Behavior change for reviewers to weigh
Fast tool-using turns now show no intermediate progress. That is the intent, but it is a default-behavior change: someone who liked watching each round will see less. The receipt (#4) is the compensating signal.
How was this tested?
pnpm lint— clean.pnpm test— 699 files, 13091 passed, 14 skipped.pnpm audit:env:check,pnpm scan:env:check,pnpm audit:sdk:check— all pass.pnpm build— clean.streaming.test.ts: latency gate (asserts the production default, no override), rolling window, retry-boundary regression, dead-turn guard, receipt present / absent / not-recorded, suggestion dedupe, and pure-helper unit tests.progressDelayMs: 0because they assert progress rendering rather than gating; the assertions themselves are unchanged. No assertion was weakened — when the receipt initially tripped thenot.toContain('◦')invariant, the receipt's marker was changed to⏱️rather than relaxing the test.Not tested: live Telegram delivery (no integration harness); the 5s gate is exercised via the injectable override plus the default-path test.
Checklist
pnpm lintpassespnpm testpassesgit commit -s) per the DCO