feat(plugins): add register_telegram_handler plugin API (mirrors Slack) - #61576
paoloantinori wants to merge 6 commits into
Conversation
Related: #59159 (earlier open PR, 2026-07-05) adds the same |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding a focused plugin extension and tests. I found one blocking PTB compatibility issue.
Problems
hermes_cli/plugins.py:1087-1088requireshandler.handle, but Hermes pinspython-telegram-bot[webhooks]==22.6inpyproject.toml:161; PTB v22.6'sBaseHandlercontract usescheck_update()andhandle_update(). Consequently, the documentedTypeHandler(...)example is rejected before registration. The fake intests/gateway/test_telegram_plugin_handlers.pymasks this by aliasinghandle = handle_update.- The existing MEMBER comment identifies #59159 as an earlier open implementation of the same public method using a lazy
(application, adapter)factory. These are incompatible API shapes under the same name and need a canonical maintainer choice.
Suggested changes
- Validate
handle_updateand add a regression case without ahandlealias. - Consolidate with #59159's chosen API shape, then add a connect-path test that records
Application.add_handler(..., group=...).
Automated hermes-sweeper review.
| # that don't use Telegram (it is an optional dependency). | ||
| if not ( | ||
| callable(getattr(handler, "check_update", None)) | ||
| and callable(getattr(handler, "handle", None)) |
There was a problem hiding this comment.
Blocking: Hermes pins python-telegram-bot 22.6, whose BaseHandler API exposes handle_update, not handle. This rejects real TypeHandler/MessageHandler instances, including the example above. Validate handle_update instead and make the test stand-in match that real interface.
|
Thanks @teknium1, @alt-glitch — good catches. Blocking bug: fixed in Connect-path test: added in On consolidation with #59159: fair flag, and #59159 is earlier — happy to defer to maintainers on the canonical shape. For context on why I opened this separately: I modeled it on Broader design note (not blocking): neither an instance nor a factory fully covers adapter-coupled plugin needs — e.g. a handler that fires on an inbound reaction and wants the content the bot sent for that message has to intercept outbound |
|
Following up to drive the consolidation the earlier review flagged. State: #59159 now reports Per the suggestion to align on #59159's factory shape, I'm happy to rebase this PR onto that API — plugins pass a
I'll start that rebase in the next day or so unless you'd rather revive #59159 directly — in which case I'll close this one. Either way, one word steers it. |
|
Done — rebased onto #59159's factory shape in
This supersedes #59159 — happy to see it closed (it conflicts with main and has been stale since Jul 5). Also fixed a doc bug carried over from #59159: the Ran |
Mirrors register_slack_action_handler so plugins can react to Telegram updates the built-in handlers don't cover (message reactions, edited messages, custom commands, chat-member changes, ...). - PluginContext.register_telegram_handler(handler, group=0): duck-types a python-telegram-bot BaseHandler (check_update/handle — PTB stays an optional dependency) and queues a (handler, group, plugin_name) tuple. - PluginManager._telegram_handlers + get_telegram_handlers() accessor, and clear() on forced re-discovery (parallels _slack_action_handlers). - TelegramAdapter.connect() wires registered handlers into the PTB app at connect time, exactly mirroring how the Slack adapter consumes register_slack_action_handler. A single bad handler (or a plugin-layer failure) is logged and skipped; gateway connect is never blocked. Motivating use case: a plugin that re-renders a pipe-table reply as a PNG photo when a user reacts thumbs-down — Telegram Web can't render native rich tables (sendRichMessage -> "not supported" card), so the plugin renders an image fallback on demand. With this API that plugin registers its reaction handler cleanly instead of monkey-patching the adapter.
register_telegram_handler duck-typed on check_update + `handle`, but the pinned python-telegram-bot 22.6 BaseHandler contract is check_update() + handle_update() (no `handle` attribute). The docstring's own TypeHandler(...) example was rejected before registration; only the test fake's `handle = handle_update` alias masked it. - Validate handle_update instead of handle. - Drop the masking alias so _FakeHandler mirrors a real PTB v22 handler. - Add a regression test asserting no `handle` attr is required. Addresses review feedback on NousResearch#61576.
Extract the plugin-handler wiring out of TelegramAdapter.connect() into _wire_plugin_handlers() (pure code motion, identical behavior) so it can be exercised without running a full PTB connect(). Add TestTelegramAdapterPluginHandlerWiring covering the connect path that get_telegram_handlers() feeds: - each queued handler is Application.add_handler()'d with its group - an empty queue is a no-op - a plugin-manager load failure is isolated (connect stays safe) - one rejected handler does not block the rest from wiring Addresses review on NousResearch#61576 (asked for a connect-path test recording Application.add_handler(..., group=...)).
… shape Rebases the plugin API onto NousResearch#59159's factory shape per review feedback ("Consolidate with NousResearch#59159's chosen API shape"): - register_telegram_handler(factory): the adapter invokes each factory with (application, adapter) at connect time, right after the PTB Application is built and BEFORE the core handlers are added — so pattern-scoped plugin handlers take precedence while everything else falls through to core. Plugins construct their own PTB handlers inside the factory, so the check_update/handle_update duck-type validation and the group param are gone; PTB's own contract applies at add_handler. - PluginManager: _telegram_handler_factories / get_telegram_handler_factories (was the (handler, group, plugin_name) tuple queue). - TelegramAdapter._wire_plugin_handlers: invokes factory(self._app, self); load-failure and per-factory exceptions isolated so one bad plugin can't break connect. NousResearch#59159 can now close as superseded. Tests rewritten for the factory shape: queuing/validation, the connect-path wiring (factories invoked with (app, adapter)), and isolation (load failure + a raising factory don't block others). Also fixes a doc bug carried over from NousResearch#59159: the Notes bullet advertised an adapter.bot attribute that doesn't exist (only self._bot); corrected to point plugin authors at application.bot (PTB). Ran code-review (high) + simplify before pushing.
… registrations Post-rebase quality pass (/simplify, 4 reviewers): - _wire_plugin_handlers(app) is now the first statement of _register_handlers instead of a call that had to be mirrored at every Application build site. The plugin/core/observer lockstep guarantee becomes structural: a future build site cannot forget the pairing. - register_telegram_handler now goes through the ownership ledger (_track + _remove_identity) like its Slack sibling, returning a PluginRegistration: a reloaded plugin's old factory is removed on unload instead of being re-wired forever. - Dropped the test file's degraded local telegram mock (weaker setdefault shape; the gateway conftest installs the comprehensive one at collection time), folded the None guard test into the non-callable one, and removed a no-op list comprehension in an assertion. Added a teardown test for the registration handle. - Trimmed manager-side comments to entry shape; the precedence/ isolation contract now lives in the two docstrings that own it.
Addresses the /code-review high findings on the rebased branch: - Async factories are rejected at registration (inspect.iscoroutine- function) instead of silently no-opping: wiring runs from synchronous code, so an async def factory's coroutine could never be awaited. A sync-callable factory that still returns a coroutine gets it closed with an ERROR log at wire time, never a GC-time RuntimeWarning. - A factory that adds group-0 handlers is flagged with a WARNING naming the plugin: group 0 is shared with the core handlers and PTB dispatches only the first match per group, so an unscoped handler silently swallows the core button/text flows. - Tests pin the plugin-before-core order inside _register_handlers (a reorder would otherwise stay green while scoped plugin handlers lose first-match to the core set), plus coverage for the new guards. - The factory contract is now documented in full: sync-only, idempotent across transient-init rebuilds, dispose dequeues future connects but does not unwire a running Application. - New "React to Telegram updates" section in the plugin developer guide mirroring the Slack action-handler docs, including the pattern= scoping rule. - Removed the em-dash from authored docstrings/comments.
cec03dc to
1d54a0b
Compare
|
Known limitation tracked in #87770: factories registered after the adapter has connected (late plugin load or force-discovery from tool paths) are queued but not wired onto the running Application until a reconnect. The Slack action-handler registry shares the same shape. Out of scope for this PR; the fix needs a registration-change notification from the plugin manager and should cover both adapters. |
|
Thanks @paoloantinori — this exact surface landed in #59217 (34393c3): ctx.register_telegram_handler exists as a back-compat alias over the generic ctx.register_platform_handler("telegram", factory), wired into the PTB Application at connect() before core handlers. Your factory-based handlers port directly. Closing as implemented on main. |
What
Adds
PluginContext.register_telegram_handler(handler, group=0)and the matchingPluginManager.get_telegram_handlers(), wired into the PTBApplicationatTelegramAdapter.connect()time. This is the Telegram equivalent of the existingregister_slack_action_handler/get_slack_action_handlers()pair — it fills an asymmetric gap (Slack exposed one, Telegram did not).Why
Plugins currently have no official way to react to Telegram updates the built-in handlers don't cover — message reactions (
message_reaction), edited messages, chat-member changes, custom commands. The only options today are monkey-patching the adapter or asys.meta_pathimport hook, both of which couple to internals that change between releases.register_slack_action_handleralready solves this for Slack (SlackAdapterwires registered handlers into itsslack_bolt.AsyncAppat connect). This PR gives Telegram the symmetric treatment so plugins register atelegram.ext.BaseHandleronce and the adapter adds it to the app at connect — no monkey-patching, no coupling to the adapter's internal import/lifecycle.How
PluginContext.register_telegram_handler(handler, group=0)— duck-types aBaseHandlerviacheck_update/handle(sopython-telegram-botstays an optional dependency — no import inplugins.py), then queues(handler, group, plugin_name).PluginManager._telegram_handlers+get_telegram_handlers()(returns a copy), cleared on forced re-discovery — parallel to_slack_action_handlers.TelegramAdapter.connect()adds each registered handler viaself._app.add_handler(handler, group=g). A single bad handler (or a plugin-layer failure) is logged and skipped; gateway connect is never blocked — same defensive shape as the Slack wiring.Usage
Use
group=1(non-zero) to run alongside the built-in group-0 handlers without displacing them — documented in the docstring.Tests
New
tests/gateway/test_telegram_plugin_handlers.py(7 tests, no PTB dependency — duck-typed handler stand-in) covering validation, queuing, group, accessor-copy semantics, multi-plugin ordering, and the(handler, group, plugin_name)tuple contract the adapter loop unpacks.Notes
register_platform_handler) is deliberate and matches the existing house style: PTBadd_handler(handler, group)and slack_boltApp.action(id)(fn)have incompatible signatures/semantics, so a generic API would need**kwargs+ per-platform parsing with no caller saved. Only Slack and Telegram consume plugin handlers today.connect()rebuildsself._appon each call, so handlers are registered exactly once per connect (no duplication on reconnect) — same property the built-in handler block already relies on.Checklist
register_slack_action_handler(PluginContext + PluginManager + adapter wiring)python-telegram-botoptional/code-review highand/simplifyrun before opening