Skip to content

fix(telegram): acknowledge unhandled callback queries - #78874

Open
Kailigithub wants to merge 2 commits into
NousResearch:mainfrom
Kailigithub:fix/issue-78788-acknowledge-telegram-callbacks
Open

fix(telegram): acknowledge unhandled callback queries#78874
Kailigithub wants to merge 2 commits into
NousResearch:mainfrom
Kailigithub:fix/issue-78788-acknowledge-telegram-callbacks

Conversation

@Kailigithub

Copy link
Copy Markdown
Contributor

Summary

  • Acknowledge Telegram callback queries with no data so the client loading indicator is cleared.
  • Acknowledge callback data that does not match a built-in Hermes namespace, including stale buttons.
  • Preserve all existing built-in callback handling.
  • Complement the plugin callback-handler work in feat(telegram): plugin callback-query handler registration #78619: callbacks that no handler consumes still receive an acknowledgement.

Verification

  • python3 -m py_compile plugins/platforms/telegram/adapter.py tests/gateway/test_telegram_clarify_buttons.py
  • uv run --extra dev --extra messaging python -m pytest tests/gateway/test_telegram_clarify_buttons.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_telegram_thread_fallback.py tests/gateway/test_telegram_callback_auth_fail_closed.py -q — 39 passed
  • Regression test fails on the unfixed source and passes with this change (git-stash verification)
  • git diff --check

Closes #78788

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 4, 2026
@spfcraze

spfcraze commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

The unconditional await query.answer() at the update_prompt: fall-through (adapter.py:6653-6654) will answer twice the callbacks that #78619's plugin handlers consume, since those handlers answer the query themselves.

Problems:

  • The ack is unconditional: the diff never reads the result of the plugin dispatch that feat(telegram): plugin callback-query handler registration #78619 inserts into this same block (await self._dispatch_plugin_callback_handlers(query, data), adapter.py:6651-6663 in its diff), whose documented handler shape is await query.answer(); ...; return True. A second answerCallbackQuery on the same query returns 400 ("query is too old and response timeout expired or query ID is invalid"), which PTB raises as TelegramError.
  • Composition: if the ack runs first, the handler's own query.answer() is the second answer and fails; feat(telegram): plugin callback-query handler registration #78619's dispatch wrapper catches the raise, logs "callback handler raised", and counts the update consumed — the handler's post-answer work never runs. If the dispatch runs first, the unconditional ack is the second answer and fails on every consumed tap.

Suggested changes:

  • Gate the ack on the dispatch result, as the description's "no handler consumes" wording describes: if not await self._dispatch_plugin_callback_handlers(query, data): await query.answer() — ack only the callbacks no plugin handler consumed.

Checked against acfb985c5 — the PR head when this was written — and 43717123c, main at the same moment. Verified mechanically: the diff grep and the PR description at the former; the code on main at the latter. The reading of intended scope is inference. If I have misread the intent here, please say so.

Forward-compatible with NousResearch#78619: if _dispatch_plugin_callback_handlers
is registered (post-NousResearch#78619 merge), the ack is only emitted when the
plugin dispatch returns False (no handler consumed). Plugin handlers
that already call query.answer() themselves no longer see a second
answerCallbackQuery that PTB rejects with a 400.

When the dispatch method is absent (current main), behavior is
unchanged from the prior fix: the unconditional ack clears the
client loading indicator.

Adds regression test covering the gated-on-dispatch path.
@Kailigithub
Kailigithub force-pushed the fix/issue-78788-acknowledge-telegram-callbacks branch from acfb985 to 572b3da Compare August 8, 2026 19:08
@Kailigithub

Copy link
Copy Markdown
Contributor Author

Thanks for the close read — your point about the double- race with #78619 is correct as a forward-compat concern, and I've just pushed a follow-up commit that addresses it.

Update on the head: the new commit gates the fall-through ack on when that method is registered:

dispatch = getattr(self, "_dispatch_plugin_callback_handlers", None)
if dispatch is None:
    await query.answer()
else:
    if not await dispatch(query, data):
        await query.answer()

Behavior matrix:

#78619 state unknown callback path
Not merged (current main) unconditional ack (unchanged from prior commit)
Merged, plugin handler returns True dispatch consumes, no second answerCallbackQuery
Merged, plugin handler returns False dispatch declines, adapter answers

The new regression test test_unknown_callback_plugin_dispatch_consumes_skips_ack injects a fake dispatch that calls query.answer() and returns True; the assertion is query.answer.await_count == 1. Verified that the test fails on unpatched main (await_count == 0) and passes on the patched head.

CI rerun queued — full tests/gateway/test_telegram_clarify_buttons.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_telegram_thread_fallback.py tests/gateway/test_telegram_callback_auth_fail_closed.py slice passes locally (40/40).

Closes #78788

@Kailigithub

Copy link
Copy Markdown
Contributor Author

Minor correction to my previous comment (bash ate some backticks):

The reviewer's concern is specifically about the double-answer()** race** with #78619's plugin handlers — when a plugin handler already calls query.answer(), my prior unconditional ack in the same callback would issue a second answerCallbackQuery that PTB rejects with HTTP 400.

The gating approach above (commit on branch tip) eliminates that race for the post-#78619 state while preserving the unconditional ack for the current main state. Thanks again for the careful review.

@andrexibiza

Copy link
Copy Markdown
Contributor

Campaign interlock: this PR is tracked by Telegram Revolution EPIC #78791 as the callback-query fix candidate. Existing authorship and useful work remain preserved; implementation acceptance is adjudicated separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(telegram): answer unknown/data-less callback queries — client spinner never resolves

4 participants