Skip to content

feat(plugins): pre_callback_dispatch hook for Telegram callback queries - #34539

Closed
sh940701 wants to merge 1 commit into
NousResearch:mainfrom
sh940701:pr/pre-callback-dispatch-hook
Closed

feat(plugins): pre_callback_dispatch hook for Telegram callback queries#34539
sh940701 wants to merge 1 commit into
NousResearch:mainfrom
sh940701:pr/pre-callback-dispatch-hook

Conversation

@sh940701

Copy link
Copy Markdown

What does this PR do?

Adds a new plugin hook pre_callback_dispatch that fires at the top of TelegramAdapter._handle_callback_query for every inbound callback query (inline-keyboard button click), before any built-in prefix branch (model picker, gmail triage, exec approval, slash confirm, clarify, retirement notices, …) runs.

Today there is no plugin hook for inbound callback queries. pre_gateway_dispatch fires only on MessageEvents; inline-keyboard clicks hit _handle_callback_query directly and the hard-coded prefix branches handle them. There is no way for a plugin to:

  • Retire a legacy callback-data prefix with a custom user-visible notice instead of the built-in answer text.
  • Inspect a click pre-dispatch for telemetry / auth / abuse mitigation.
  • Rewrite the callback data field before the built-in branches see it.

This hook mirrors the action semantics of pre_gateway_dispatch, so plugins that already speak skip / rewrite / allow get a consistent surface for the callback path.

Action semantics

{"action": "skip",    "reason": "...", "answer_text": "..." (optional)}  # drop click; if
                                                                          # answer_text present,
                                                                          # core awaits
                                                                          # query.answer(text=...)
                                                                          # so sync hooks can answer
{"action": "rewrite", "data":   "..."}                                    # replace query.data
{"action": "allow"}   /   None                                            # normal dispatch

Result precedence: hooks are invoked in plugin-registration order; the FIRST result whose action is skip / rewrite / allow wins. Later results for the same callback are ignored. Plugins SHOULD return None for cases they don't handle so they do not accidentally veto a later plugin's intent.

Kwargs delivered: query: telegram.CallbackQuery (live PTB object), data: str (current payload, possibly already rewritten by an earlier hook), gateway: GatewayRunner | None (None in unit-test contexts or pre-adapter-registration), source: SessionSource (chat_id may be "" if the callback's chat can't be resolved; chat_type defaults to "dm").

Exception handling: ordinary Exception raised by any callback (or by the invocation layer) fails open — dispatch falls through to the normal prefix branches. Process-control exceptions (KeyboardInterrupt, SystemExit, asyncio.CancelledError, other BaseException subclasses) propagate unchanged.

Security note

The hook fires BEFORE the built-in prefix branches, which means a malicious or buggy plugin can intercept and answer ANY callback (including unauthenticated ones). This is documented in the new contributor docs. The behavior is symmetric with pre_gateway_dispatch and intentional — plugins that need to act on legacy callbacks must run before the built-in retirement notices.

Related Issue

Fixes #

(No related issue — opening this PR to start that conversation.)

Type of Change

  • ✨ New feature (non-breaking change that adds functionality)

Changes Made

  • gateway/platforms/telegram.py — invoke pre_callback_dispatch at the top of _handle_callback_query; honor skip/rewrite/allow; when answer_text is present, await query.answer(text=...) so synchronous hooks can answer.
  • gateway/platforms/base.pyBasePlatformAdapter gains an optional _gateway_ref back-reference + set_gateway_ref() setter so the callback path can pass a live GatewayRunner to hooks.
  • gateway/run.py — wire adapter.set_gateway_ref(self) at both connect sites (initial bring-up + reconnection watcher).
  • hermes_cli/plugins.py — register the new hook name; pass it through the plugin loader.
  • tests/gateway/test_pre_callback_dispatch.py — new file, 14 unit tests covering: clean allow, skip without answer, skip with answer_text, rewrite of data, hook exception fail-open, precedence (first-wins), invocation-layer exceptions, BaseException propagation, kwargs shape.
  • tests/hermes_cli/test_plugins.py — extended for the new hook name.

How to Test

  1. From a clean working tree: pytest tests/gateway/test_pre_callback_dispatch.py tests/hermes_cli/test_plugins.py -q — 88 tests pass on this branch.
  2. Full suite: pytest tests/ -q — passes (no regressions; result attached below if requested).
  3. Manual: write a tiny plugin that returns {"action": "skip", "reason": "test", "answer_text": "hi"} for any callback data; install it; click any inline-keyboard button — Telegram shows "hi" instead of the built-in branch's response.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (feat(plugins):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this feature
  • I've run pytest tests/ -q and the suite passes
  • I've added tests for my changes (tests/gateway/test_pre_callback_dispatch.py — 14 tests)
  • I've tested on my platform: macOS 15.5 (Darwin 25.5.0), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation — N/A (hook surface; in-code docstrings are present, happy to add a section to docs/PLUGINS.md if the maintainer wants it)
  • cli-config.yaml.example — N/A (no new config keys)
  • CONTRIBUTING.md / AGENTS.md — N/A (no architecture/workflow change beyond the new hook name)
  • Cross-platform considered — pure async/dict API, no platform-specific calls added
  • Tool descriptions/schemas — N/A (no tool changes)

Notes for reviewer

  • The hook intentionally mirrors pre_gateway_dispatch action semantics (skip / rewrite / allow). If a different shape is preferred for callback queries (e.g. answer_callback_query instead of answer_text), happy to rename in this PR.
  • I have a downstream plugin (legacy callback-data retirement notices) that depends on this hook today and is currently using a monkey-patch as a stop-gap. Landing this hook removes the monkey-patch.

Mirror of pre_gateway_dispatch action semantics for the inline-keyboard
click path. Fired at the TOP of TelegramAdapter._handle_callback_query
for every inbound callback query, BEFORE any built-in prefix branch
(model picker, gmail triage, exec approval, slash confirm, clarify, …)
runs.

Plugins return action dicts to influence flow:
  {"action": "skip",    "reason": "...",
   "answer_text": "..." (optional)}                # if present, core awaits
                                                   # query.answer(text=...)
                                                   # before returning so sync
                                                   # hooks can answer callbacks
  {"action": "rewrite", "data":   "..."}           # replace query.data, continue
  {"action": "allow"}   /   None                   # normal dispatch

Result precedence: hooks are invoked in plugin-registration order; the
FIRST result whose action is skip/rewrite/allow wins. Later results for
the same callback are ignored.

Kwargs: query: telegram.CallbackQuery (live PTB object), data: str,
gateway: GatewayRunner | None (None outside a real gateway runner),
source: SessionSource.

Exception handling: ordinary Exception fails open — dispatch falls
through to normal prefix branches. Process-control exceptions
(KeyboardInterrupt, SystemExit, asyncio.CancelledError, other
BaseException subclasses) propagate unchanged.

Auth note: hook runs BEFORE per-branch authorization, so plugins
handling unauthorized clicks (e.g. retiring a legacy callback prefix
with a "reply by text" notice) can answer without triggering an
"Unauthorized" toast. Plugins that want auth must check source.user_id
themselves.

Rewrite warning: the model-picker branch (mp:/mm:/mb/mx/mg:) has NO
authorization check today; a plugin's rewrite that produces one of
those payloads will route the click into the model picker with
attacker-controlled bytes. Plugins SHOULD prefer skip with
plugin-owned handling for legacy migrations. A regression test pins
current behavior so future allow-list hardening is opt-in and explicit.

Logging: skip's data_prefix uses a bounded prefix extraction
(``data.split(":", 1)[0]`` only when a colon is present, else
``"<no-prefix>"``) so an opaque callback id without a delimiter is
never logged in full.

Implementation:
- VALID_HOOKS gains "pre_callback_dispatch" with full contract,
  precedence, rewrite warning, and exception handling documented inline.
- BasePlatformAdapter gains ``self._gateway_ref`` + ``set_gateway_ref``
  so callbacks fired from inside the adapter can pass a gateway handle.
  May stay None in unit-test contexts.
- GatewayRunner calls ``adapter.set_gateway_ref(self)`` at both the
  initial registration site and the reconnection site.
- TelegramAdapter._handle_callback_query invokes the hook at top,
  processes skip/rewrite/allow, awaits ``query.answer(text=answer_text)``
  on skip when payload contains answer_text (also fail-open), falls open
  on any error.

Tests:
- tests/hermes_cli/test_plugins.py: VALID_HOOKS membership +
  action-dict collection sanity (skip-shape).
- tests/gateway/test_pre_callback_dispatch.py (new): 14 cases covering
  skip / rewrite / allow / None-return / exception / garbage-return /
  kwargs-contract / runs-before-auth-check / skip-with-answer_text /
  skip-answer-failure / skip-without-answer_text / first-actionable-wins /
  rewrite-into-model-picker-permitted / skip-log-bounded-prefix.
@sh940701
sh940701 force-pushed the pr/pre-callback-dispatch-hook branch from 61f5bcc to 86bc4c1 Compare May 29, 2026 10:07
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter labels May 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #21469 (feature request) and competing with #21471 (implementation PR by different author). Same feature: pre_callback_dispatch plugin hook for Telegram callback queries. #21471 was triaged on 2026-05-07 and is still open.

@sh940701

Copy link
Copy Markdown
Author

Thanks @alt-glitch for the quick triage — apologies for the duplicate. I missed #21471 in my pre-submit search (searched for pre_callback_dispatch rather than callback_query).

Closing this PR in favor of #21471 (@ChaseFlorell's earlier implementation). I'll move the design notes from this PR over there as a comment — there are a few additive surface ideas (rewrite action, answer_text skip payload, gateway back-ref) that may or may not be worth folding into the simpler hook in #21471. Happy to defer entirely to the maintainer and @ChaseFlorell on whether any of that is useful or out-of-scope.

PR #34541 (streaming stale-timer gate) is unrelated and I'll leave that one open.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants