Repository navigation
fix(telegram): require callback chat context - #6871
Conversation
b82bd1f to
c213331
Compare
The explanatory comment about populating the canonical top-level id was accidentally moved into message_event during the callback_event fix, where it was self-referential (message_event already set message_id unconditionally before this PR and needed no such note). Move it back next to the .message_id(message_id) call in callback_event where it actually applies, and rewrap it one sentence per line per the repo's prose convention.
|
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 @@ | |||
| - Dropped Telegram callback events without chat context instead of routing them into an empty synthetic channel. (@houko) | |||
There was a problem hiding this comment.
changelog.d/README.md format rule: the fragment body must NOT start with a leading - — cargo xtask collect-fragments prepends its own - when assembling ## [Unreleased], so this will render as - - Dropped.... Please drop the leading - . Also missing the trailing (#6871) reference before (@houko) (not tooling-enforced, but prevents a duplicate generated line in the release notes).
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated correctness/CLAUDE.md review pass on top of the earlier round (Claude-attribution and changelog-format findings from the prior pass are already addressed/flagged — not repeating them). Verified the fail-closed guard is consistent with existing code, both accept/reject paths are tested, and inline-mode callbacks are not a regression. Left one comment on a cross-SDK parity gap for a maintainer call — see inline. cargo check, cargo test -p librefang-sidecar-telegram, and cargo clippy -p librefang-sidecar-telegram --all-targets -- -D warnings all pass clean on this branch. No mechanical fixes applied — nothing met the ≥90%-confidence bar for a direct commit.
Generated by Claude Code
| pub fn callback_event(cq: &CallbackQuery) -> Option<Value> { | ||
| let user = cq.from.as_ref()?; | ||
| let message = cq.message.as_ref()?; | ||
| if message.chat.id == 0 { |
There was a problem hiding this comment.
Verified: this mirrors the existing msg.chat.id == 0 guard already established in message_event (line 331, same file, pre-existing), so the fail-closed behavior here is consistent with the rest of the translator rather than a new invention.
Both directions are covered by the new test: missing message returns None, and message present with a degenerate/zero chat.id also returns None — while the pre-existing callback_event_emits_message_id_as_a_string_in_both_slots test covers the accept path with a real chat.id.
Inline-mode callbacks (Telegram sends inline_message_id instead of message for those) aren't a regression here: CallbackQuery (api/types.rs) never modeled inline_message_id in the first place, so those updates already had message: None and were being mis-routed to an empty synthetic channel before this fix, not correctly handled. Dropping them now is strictly safer than the prior behavior, not a new gap.
Cross-SDK parity gap worth a maintainer decision, not a bug in this PR (file untouched by this diff): sdk/python/librefang/sidecar/adapters/telegram.py's _callback_to_event (~line 1344-1349) only checks if not message: return None and if not isinstance(chat_id, int): return None — it has no equivalent to the chat.id == 0 guard this PR adds on the Rust side. 0 passes isinstance(chat_id, int), so a callback with a degenerate zero chat id would still route to channel_id="0" in Python, reproducing the exact synthetic-channel bug this PR fixes in Rust.
This asymmetry isn't new or specific to callbacks, either — Python's plain-message path (_update_to_event, ~line 1481-1483) only checks chat_id is None, never chat_id == 0, so the general message_event-equivalent guard (translator.rs line 331) also has no Python counterpart. Real Telegram traffic never sends chat id 0, so this only matters for malformed/spoofed webhook payloads, which is presumably why it wasn't caught by existing parity tests.
Should the same zero-id guard land in the Python adapter (this PR or a fast-follow), or is Python intentionally out of scope here? Leaving as a question rather than a diff since it's a different SDK/language and outside this PR's touched files.
Generated by Claude Code
| @@ -531,13 +525,10 @@ pub fn callback_event(cq: &CallbackQuery) -> Option<Value> { | |||
| .channel_id(chat_id) | |||
| .platform("telegram") | |||
There was a problem hiding this comment.
Reviewed the core logic independently and it holds up: callback_event correctly fails closed on both a missing message and a degenerate chat.id == 0, consistent with the pre-existing guard in message_event (line 331); confirmed update_to_event (line 607) propagates the None straight through rather than substituting a fallback channel. cargo test -p librefang-sidecar-telegram --bins (63 passed), cargo clippy --bins --all-targets -- -D warnings, and cargo fmt --check all clean on current HEAD.
Separate from the logic: commit ca89f729c ("fix: relocate misplaced message_id builder comment to callback_event") on this branch has author identity Claude <noreply@anthropic.com> — same class of issue already flagged on PR #6868 (commit 8568134b4) and #6870 (commit aa6170be7). The commit's content is fine (a correct, harmless comment relocation), but the identity itself violates the repo's no-AI-attribution rule and will need the same rebase/re-author treatment before merge. Flagging rather than fixing since correcting it requires a history rewrite this session isn't able to force-push.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated review pass: found an AI-authored commit (author identity) in this branch's history, flagged inline for maintainer judgment since fixing it needs a history rewrite I won't force-push. The functional change itself looks correct and well-tested.
Generated by Claude Code
There was a problem hiding this comment.
Flagging for maintainer judgment rather than editing directly (fixing requires rewriting history, which I won't force-push):
Commit ca89f729 ("fix: relocate misplaced message_id builder comment to callback_event") on this branch has author identity Claude <noreply@anthropic.com>. CLAUDE.md's "No AI / Claude attribution" section is explicit that a commit whose author identity resolves to Claude/Anthropic is disallowed even when the message text itself is clean — this is exactly what the repo's own commit-msg hook checks via GIT_AUTHOR_IDENT. I can't fix this without rewriting the branch's history and force-pushing, which I'm not permitted to do. Recommend rebasing to correct the author identity on this commit before merge.
The substantive change itself (drop callback events with no chat context, require message.chat.id != 0) looks correct and is covered by callback_event_requires_chat_context. No other findings.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated pass on head d01434da1e1f7ebaea787d6750bb5789e0819aa6.
Logic: callback_event fails closed before any dispatch — cq.message.as_ref()? then chat.id == 0 guard both run before content/metadata are built, matching the pre-existing message_event pattern (no prior panic path existed here; the bug being fixed was silent misrouting to an empty synthetic channel, not a crash). update_to_event's unconditional answer_callback_query spawn only needs cq.id and doesn't bypass the guard. callback_event_requires_chat_context covers both the realistic case (no message, e.g. inline-mode callbacks) and the defensive chat.id == 0 case.
Changelog: changelog.d/fixed/6871-telegram-callback-channel-id.md format is already corrected (no leading - , trailing (#6871) (@houko)).
One outstanding item, consolidated below rather than repeated in full.
Generated by Claude Code
There was a problem hiding this comment.
Status (previously flagged 3x on this thread, consolidating rather than repeating): commit ca89f729c is still authored as Claude <noreply@anthropic.com>, unchanged since the last pass. Still blocking on a maintainer rebase (--reset-author or squash-on-merge) before this can land; no new detail to add.
Generated by Claude Code
Summary
Verification
cargo test --manifest-path sdk/rust/librefang-sidecar-telegram/Cargo.toml callback_event_requires_chat_contextcargo test --manifest-path sdk/rust/librefang-sidecar-telegram/Cargo.toml --all-targetscargo clippy --manifest-path sdk/rust/librefang-sidecar-telegram/Cargo.toml --all-targets -- -D warningsgit diff --check