Repository navigation
fix(telegram): budget dynamic close suffixes - #6859
Conversation
8ebe32e to
5074a26
Compare
Both sides bound the emitted chunk against carry plus generated close tags, at different points. Keep main's degenerate-branch escape hatch and run this branch's exact-suffix shrink loop after it, so the non-degenerate path is covered too; the anchor-carry regression keeps the wider limit that satisfies both.
New comments added by this PR were hard-wrapped at a fixed column; CLAUDE.md's prose-wrapping rule requires breaking only at sentence boundaries. No code changes.
|
Generated by Claude Code |
houko
left a comment
There was a problem hiding this comment.
Automated CLAUDE.md compliance check: one changelog fragment formatting nit below.
Generated by Claude Code
| @@ -0,0 +1 @@ | |||
| - Enforced Telegram's UTF-16 chunk limit against the actual generated HTML close-tag suffix instead of relying only on a fixed reserve. (@houko) | |||
There was a problem hiding this comment.
changelog.d/README.md: not tooling-enforced, but ending the bullet with (#6859) before (@houko) avoids cargo xtask release generating a duplicate - <PR title> (#6859) (@houko) line for this same PR in the release notes.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Mechanical CLAUDE.md compliance pass. Correctness of the budget/close-suffix fix verified locally (21/21 format::chunk tests pass, including the new exact-suffix regressions). Changelog fragment formatting fixed and pushed. One commit-author-identity issue flagged inline for maintainer action (cannot rewrite history from this pass).
Generated by Claude Code
| fn tag_carry_preserves_anchor_href_with_attributes() { | ||
| // Anchor with attributes — the carry must preserve `href="..."` verbatim when reopening. | ||
| let s = format!("<a href=\"https://example.com\">{}</a>", "x".repeat(40)); | ||
| // The opening tag (30 units) plus its required `</a>` suffix must fit before the chunker can preserve formatting. |
There was a problem hiding this comment.
Commit author-identity check (CLAUDE.md "Commit & PR hygiene" / commit-msg hook rule): commit b54046e8 ("docs: reflow new chunk.rs comments to one sentence per line") — the commit that touches this line — has author identity Claude <noreply@anthropic.com>. That resolves to an AI author identity, which the repo's commit-msg hook is meant to reject server-side via git var GIT_AUTHOR_IDENT. Since history on this branch can't be rewritten/force-pushed from this review pass, flagging for the maintainer to rebase and re-author that one commit (e.g. git rebase -i + git commit --amend --author="houko <suzukaze.haduki@gmail.com>" on just that commit) before merge.
Generated by Claude Code
…l emoji filler split_to_utf16_chunks now has an exact close-suffix budget check, but every regression test for it used ASCII filler. Telegram's real-world content for this exact code path — tg-emoji custom-emoji wrappers — always contains an astral-plane surrogate pair as the fallback glyph, so the shrink loop's UTF-16 accounting needs a non-ASCII case to actually exercise 2-unit-per-scalar boundary math.
| close_suffix.clear(); | ||
| next_carry.clear(); | ||
| } | ||
| while utf16_len(&emitted_text).saturating_add(utf16_len(&close_suffix)) > limit { |
There was a problem hiding this comment.
Verified this exact-budget shrink loop with a 20k-iteration randomized fuzz (random tag nesting depth 1-5, mixed ASCII/astral filler, limits 1-40) — never overshoots. The math looks solid.
One thing I noticed while reviewing this file that's out of scope for this PR but worth flagging: the sibling Python sidecar's chunker (sdk/python/librefang/sidecar/adapters/telegram.py::_split_to_utf16_chunks) has no tag-aware carry/balancing logic at all — no unclosed_tags, no reopen-on-next-chunk. It just splits on UTF-16 length + entity boundaries. That means a long formatted message can split mid-tag-pair into individually-invalid HTML:
>>> tg._split_to_utf16_chunks("<b>" + "x"*20 + "</b>", 10)
['<b>xxxxxxx', 'xxxxxxxxxx', 'xxx</b>']The first chunk has an unclosed <b>, the middle chunk silently drops the bold formatting, and the last chunk has a dangling </b> with no opener — each would be sent as its own sendMessage call under parse_mode=HTML, so Telegram's Bot API would likely reject the first one with "can't find end of the entity". This contradicts the module docstring's claim of "full parity" / "byte-exact port" of message_truncator::split_to_utf16_chunks (this very file), which has had tag-carrying since before this PR.
I didn't fix this here since it's a different file/language and a non-trivial port (~130 lines of carry/stack logic, not a mechanical change), and this PR's diff doesn't touch it. Flagging for a follow-up — happy to be told this is already tracked elsewhere.
Generated by Claude Code
| // Choose what to emit: either the entity/tag-trimmed combined slice (normal path) or, if that left no progress to make on `remaining`, the carry plus one forced unit of input. Either way we run the SAME tag-rebalancing on the emitted text so open tags from `carry` get matching close tags appended and propagate forward via `next_carry`. | ||
| // Trim only the new input. | ||
| // Inferring progress by trimming `carry + input` and subtracting carry length would couple forward progress to assumptions inside the boundary helpers. | ||
| let trimmed_input = trim_input_boundary(input_chunk); |
There was a problem hiding this comment.
Minor process note: this branch's history (git log main..HEAD) includes a commit titled fix(telegram): decouple chunk progress from carry (#6885) in addition to the #6859 close-budget fix — i.e. this PR bundles two separately-numbered fixes. Both changes look correct together (tests pass, fuzz above covers the combined behavior), so no action needed from me, just flagging in case the intent was two separate PRs.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Reviewed the new commit (06dbd03, astral-emoji regression test). Traced the exact-suffix shrink-loop arithmetic by hand against the test's numbers (2045 * 2 = 4090 UTF-16 units, tag overhead, budget shrink from 4080→4074 units in exact 2-unit/emoji steps) — the accounting is correct and the test is a genuine, non-redundant addition (Rust's UTF-8 invariant plus the scalar-aware char_indices/encode_utf16 helpers already preclude a mid-surrogate split, so this closes the coverage gap on the 2-units-per-scalar shrink path specifically). Two comment-only prose-wrap nits inline (new block introduced by this commit; not previously flagged). No correctness issues found in the new commit; the two review threads already open on this PR (b54046e author identity, Python-sidecar parity gap, #6885 bundling note) remain as previously stated — nothing new to add there.
Generated by Claude Code
| // Same shape as `long_tag_close_suffixes_are_counted_exactly` but the | ||
| // filler is an astral-plane emoji (surrogate pair, 2 UTF-16 units per | ||
| // scalar) instead of ASCII `x`, so the exact-suffix-budget shrink loop | ||
| // has to make progress in 2-unit steps and can never land mid-surrogate-pair. |
There was a problem hiding this comment.
Prose-wrapping rule (CLAUDE.md: "no column limit; break only at sentence boundaries") — this new comment block from 06dbd0303 is hard-wrapped at a fixed column instead of one sentence per line, the same issue b54046e85 already reflowed elsewhere in this file. Suggested reflow:
// Same shape as `long_tag_close_suffixes_are_counted_exactly` but the filler is an astral-plane emoji (surrogate pair, 2 UTF-16 units per scalar) instead of ASCII `x`, so the exact-suffix-budget shrink loop has to make progress in 2-unit steps and can never land mid-surrogate-pair.Generated by Claude Code
| // A surrogate-pair split would produce a byte slice that does not | ||
| // fall on a scalar boundary; `String`'s UTF-8 invariant makes that | ||
| // unrepresentable, so `split_to_utf16_chunks` returning valid | ||
| // `String`s at all (rather than panicking on an invalid `str` | ||
| // slice) already proves every boundary landed on a full `😀`. |
There was a problem hiding this comment.
Same prose-wrapping issue as the block above (one sentence per line, per CLAUDE.md): this comment is hard-wrapped mid-sentence. Suggested reflow:
// A surrogate-pair split would produce a byte slice that does not fall on a scalar boundary; `String`'s UTF-8 invariant makes that unrepresentable, so `split_to_utf16_chunks` returning valid `String`s at all (rather than panicking on an invalid `str` slice) already proves every boundary landed on a full `😀`.Both blocks are comment-only (no logic change), so this is mechanical whenever someone touches the file next.
Generated by Claude Code
…ntence per line Two comment blocks added by the astral-content regression test were hard-wrapped at a fixed column instead of breaking only at sentence boundaries, matching the earlier hard-wrap already reflowed elsewhere in this file. No code changes.
houko
left a comment
There was a problem hiding this comment.
Automated review pass — no new blocking findings beyond what is already flagged on this PR.
Applied a mechanical prose-wrap fix directly to the branch (commit 07f9758, sdk/rust/librefang-sidecar-telegram/src/format/chunk.rs): reflowed the two hard-wrapped comment blocks inside long_tag_close_suffixes_are_counted_exactly_with_astral_content (around what were lines 577 and 594) to one sentence per line, matching CLAUDE.md's prose-wrapping rule and the exact reflow already suggested on this PR's own review thread.
No logic changed.
Verified the core budgeting fix in split_to_utf16_chunks: the shrink loop and every length comparison operate on utf16_len (UTF-16 code units), not bytes, matching Telegram's UTF-16-based message limit.
The loop recomputes close_suffix from unclosed_tags after every shrink and re-checks the total against limit, so a shrink that removes a closing tag and thereby grows the required suffix is caught on the next iteration rather than accepted as final.
Each iteration strictly decreases consumed_from_input (or falls back to plain text via the reduced_consumed == 0 || reduced_consumed >= previous_consumed escape hatch), so the loop always terminates without emitting an oversized or unbalanced chunk.
long_tag_close_suffixes_are_counted_exactly and its astral-content sibling exercise this at the exact 4096-unit production boundary with two nested tg-emoji tags each, matching the multiple-nested-tags-at-the-boundary case.
No off-by-one found.
Commit author identity (CLAUDE.md commit-msg rule): b54046e8 on this branch is still authored as Claude <noreply@anthropic.com>.
This has already been flagged twice on this PR (an issue comment and an inline review comment on chunk.rs:437), so not re-flagging a third time — just confirming it remains unresolved and still needs a maintainer rebase (git rebase -i + --reset-author / commit --amend --author=... on that one commit) before merge.
This automated pass cannot fix it directly since rewriting that commit requires a force-push.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Independent boundary/budget-safety verification on the exact-suffix shrink loop, plus scoped test + clippy run. See inline comment for details.
Generated by Claude Code
| while utf16_len(&emitted_text).saturating_add(utf16_len(&close_suffix)) > limit { | ||
| // NEW_TAG_RESERVE is only a first-pass heuristic. | ||
| // Recompute the exact suffix for the selected prefix, then shrink the input until both the body and its balancing closes fit. | ||
| // Removing a closing tag can itself increase the required suffix, so repeat until the exact tag stack stabilizes within the limit. | ||
| let previous_consumed = consumed_from_input; | ||
| let available_input = limit | ||
| .saturating_sub(utf16_len(&carry)) | ||
| .saturating_sub(utf16_len(&close_suffix)); | ||
| let reduced = truncate_to_utf16_limit(&remaining[..previous_consumed], available_input); | ||
| let reduced_input = trim_input_boundary(reduced); | ||
| let reduced_consumed = reduced_input.len(); | ||
|
|
||
| if reduced_consumed == 0 || reduced_consumed >= previous_consumed { | ||
| // Carry plus its exact close suffix can consume the whole budget at tiny/adversarial limits. | ||
| // Formatting cannot be represented there, so retain progress and fall back to the selected input's plain text rather than emitting oversized wire data or looping forever. | ||
| let plain = RE_TAG.replace_all(&remaining[..previous_consumed], ""); | ||
| emitted_text = truncate_to_utf16_limit(&plain, limit).to_string(); | ||
| close_suffix.clear(); | ||
| next_carry.clear(); | ||
| break; | ||
| } | ||
|
|
||
| emitted_text.clear(); | ||
| emitted_text.reserve(carry.len() + reduced_input.len()); | ||
| emitted_text.push_str(&carry); | ||
| emitted_text.push_str(reduced_input); | ||
| consumed_from_input = reduced_consumed; | ||
| stack = unclosed_tags(&emitted_text); | ||
| close_suffix = stack.iter().rev().map(|(n, _)| format!("</{n}>")).collect(); | ||
| next_carry = stack.iter().map(|(_, full)| full.clone()).collect(); | ||
| } |
There was a problem hiding this comment.
Independent correctness pass on the exact-suffix shrink loop, focused on the two risks called out for this change: UTF-8 char-boundary panics and a negative/underflowed budget.
Boundary safety: every slice fed back into &remaining[..previous_consumed] traces back to a length produced by truncate_to_utf16_limit (scalar-boundary-safe by construction, char_indices + len_utf8) composed with trim_input_boundary (strip_mid_tag / adjust_html_entity_boundary, both trim-from-the-back prefix operations). All three preserve "valid prefix of the original &str", so consumed_from_input is always a legal split point going into this loop, on every entry path (normal, both degenerate branches, and each shrink iteration). Confirmed no panics with a 3000-trial randomized fuzz (random tag nesting depth 0-4 from {b,i,u,s,code,tg-emoji,a href=...}, filler mixing ASCII/astral emoji/raw &/partial entities/bare </>, limits 1-20 units) — all passed, every emitted chunk within limit.
Budget safety: available_input (line 288-290) and the outer budget computation above both use saturating_sub, so a carry/close_suffix that's already ≥ limit degrades to 0 rather than underflowing; truncate_to_utf16_limit(_, 0) short-circuits to "" (line 32-34), which feeds the reduced_consumed == 0 escape hatch (line 295) rather than panicking.
Termination: the loop only continues when reduced_consumed < previous_consumed strictly, and previous_consumed is bounded below by 0, so it's guaranteed to terminate (worst case via the plain-text fallback at line 295-303) — no infinite-loop risk from repeatedly-growing close-suffixes on pathological nesting.
Ran the crate's own test target (cargo test -p librefang-sidecar-telegram --bin librefang-sidecar-telegram format::chunk): 22/22 pass, including the new ASCII and astral-plane boundary regressions at the production 4096 limit. cargo clippy -p librefang-sidecar-telegram --all-targets -- -D warnings: clean.
No correctness issues found beyond what's already flagged on this PR.
Generated by Claude Code
There was a problem hiding this comment.
Automated review note (no code issues found — logic in chunk.rs checks out, all 67 crate tests pass, clippy/fmt clean on the current diff).
Flagging a repo-policy issue for the maintainer before merge: commit b54046e85d688243b19c5c34a375d1e53a8b2a5d ("docs: reflow new chunk.rs comments to one sentence per line") on this branch is authored by Claude <noreply@anthropic.com>.
CLAUDE.md's commit-msg hook is specifically designed to reject a commit whose author identity resolves to Claude/Anthropic, so this slipped through with hooks not applied (or bypassed) at commit time.
Fixing the author on an already-pushed commit needs a rebase + force-push, which is outside what an automated reviewer should do unilaterally — leaving this for a maintainer to squash/reauthor before merge.
Generated by Claude Code
Summary
tg-emojitags at the production 4096-unit boundaryVerification
cargo test --manifest-path sdk/rust/librefang-sidecar-telegram/Cargo.toml --all-targets(46 passed)cargo clippy --manifest-path sdk/rust/librefang-sidecar-telegram/Cargo.toml --all-targets -- -D warningsgit diff --checkOut of scope