feat(telegram): support secure Bot API Guest Queries - #62551
Conversation
Duplicate of #32802 (earliest open Telegram Guest Mode PR). Both implement the same Bot API 10.0 mechanism: subscribe to |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Guest Mode implementation and the dedicated regression coverage.
Problems
- Blocking —
plugins/platforms/telegram/adapter.py:3229:TypeHandler(Update, self._handle_guest_message)is registered ingroup=-1. That handler matches everyUpdate; for a normal update_handle_guest_messagereturns, but the existing text/command/media handlers are all in the default group atplugins/platforms/telegram/adapter.py:3173-3190and are no longer reached. This disables normal Telegram intake when the Guest Mode symbols are present. The related open implementation in #32802 places its catch-allTypeHandlerin group 1, after the normal handlers.
Suggested changes
- Move this handler to a later group (or make its check update-specific), keeping the generic-handler guest guards for deduplication.
- Add a handler-registration regression test covering both an ordinary text update and a guest update with Guest Mode symbols enabled.
Automated hermes-sweeper review.
|
Done. I've moved the catch-all from to a later fallback as suggested, matching the sibling PR #32802. On python-telegram-bot's group dispatch rules: Regression Coverage:
|
|
I want to add one important clarification from the history around #46196. That issue was opened specifically because Telegram guest replies still needed one-shot final delivery via Re-checking current
For So I think the actionable question now is no longer “is this a duplicate?” but rather:
Either way, the issue itself still looks open on |
You are asking how this repository should handle duplicate and consolidation status when the underlying fix is still absent from main. Case context, measured live from our triage graph (2026-07-30T11:57:01+00:00):
If you want to move this one along: keep the diff scoped, rebase onto current |
|
I rebased For the maintainer/author side: the rebase conflicts were small and localized. In my local rebase they were limited to:
So this does not look blocked by a large structural rework — it looks salvageable from a rebase/refresh standpoint. Automated validation looked good:
But a live Guest Mode repro still found two concrete issues, so I don't think this is merge-ready yet.
I also compared the rebased PR structure with an earlier local guest-mode patch I had tested against the same issue. One important difference is that this PR currently carries guest plumbing only in:
and not in:
That matches the live symptom class pretty well: the final adapter hook is there, but guest reply state is not propagated strongly enough through the runner/session layer to guarantee that every auxiliary/interim/stream path stays off normal Telegram sends. So my read is:
The two concrete follow-ups seem to be:
Happy to share the exact rebase/test setup if useful. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Verdict
Request changes. The Guest Mode plumbing itself is competent and narrowly written — the one-shot lane, the synthetic guest:<query_id> thread id, and the group-1 handler placement are all sensible, and the follow-up commit 3464527 genuinely resolves @teknium1's group=-1 blocker (verified: TypeHandler(Update, self._handle_guest_message) is now registered at group=1, adapter.py:7563, with three registration tests behind it). What blocks merge is not the mechanism but its trust boundary: this is the first code path on the Telegram gateway that lets a user the operator has never allowlisted, in a chat the bot is not a member of, dispatch a full agent turn — and after merge it is on by default with no env var, no config key and no documentation. Two blockers and five majors below are about that boundary and about the egress paths it does not yet cover; the rest are scoping and coverage points.
What I verified against head 3464527
I did not review from the diff text alone. Every claim below was checked against the head worktree and, where PTB behaviour was load-bearing, against the real python-telegram-bot 22.8 wheel (extracted, imported, telegram.__version__ == 22.8) rather than the repo's test mock — which matters, because the gateway conftest's telegram mock makes hasattr(Update, "GUEST_MESSAGE") true unconditionally and would have hidden the difference.
Reproduced in-process against the head adapter with real PTB 22.8 on sys.path:
- the handler-registration condition (
adapter.py:7545-7550) evaluates all four terms true once this PR's dependency bump is in place; 'guest_message' in Update.ALL_TYPESisTrue, and the adapter requestsallowed_updates=Update.ALL_TYPESon all four intake paths (:2031,:2141,:2543,:3310);- the guest send path's behaviour with a 9000-character payload, with
_send_path_degraded = True, and with whitespace-only content; - what
answer_guest_query's real return object does and does not carry; - which PTB filters match a guest
location/photoupdate, and whatUpdate.effective_messageresolves to for them; - the effective authorization verdict for the exact message shape the new happy-path test uses, under a strict
TELEGRAM_ALLOWED_USERS.
Test runs (venv with pytest, scripts/run_tests.sh selection):
| tree | files | result |
|---|---|---|
head 3464527, tests/gateway/test_telegram_guest_mode.py |
1 | 11 passed in 0.28s |
head 3464527, tests/gateway/test_telegram*.py |
48 | 3 failed, 1046 passed in 45.57s |
merge-base 3b2ef789d, same selection |
47 | 3 failed, 1033 passed in 45.29s |
origin/main b4f8c491d + this PR (conflicts resolved locally) |
54 | 15 failed, 498 passed in 24.33s |
bare main 466e6402f, same selection |
53 | 15 failed, 485 passed in 27.73s |
The 1,046 passed figure in the description reproduces exactly on the stale base. The 3 failures I see on the head are the same three test_telegram_thread_fallback.py tests that fail identically on the merge-base under the full-selection run and pass in isolation — order-dependent and pre-existing, not caused by this PR. The 15 failures on the merged tree are ModuleNotFoundError: No module named 'wcwidth' in my environment and are present on bare main too. Net effect of the PR on both comparisons: +13 passing, 0 new failures — exactly the 11 new guest tests plus the 2 new test_telegram_reply_mode.py cases. The suite result is genuinely clean; only the numbers in the description are stale, because main has pruned these files considerably.
Merge state
mergeable=CONFLICTING / mergeStateStatus=DIRTY. Merge base 3b2ef789d is 4441 commits behind origin/main (b4f8c491d). git merge-tree produces conflicts in exactly three files — gateway/platforms/base.py, pyproject.toml, uv.lock — while plugins/platforms/telegram/adapter.py and tests/gateway/test_telegram_reply_mode.py auto-merge clean. This confirms @abner-augusto's independently reported list, file for file. Importantly, the conflicting main state does not invalidate the PR's premises: main is still on PTB 22.6 (so the 22.6→22.8 bump is uncontested), pyproject.toml conflicts only over adjacent moved pins (starlette 1.0.1→1.3.1, slack-bolt 1.27.0→1.29.0, slack-sdk 3.40.1→3.43.0), and git grep -i 'guest_message\|answer_guest_query\|guest_query' over origin/main returns zero hits — no Bot-API-10.0 guest code exists on main, so nothing here has been superseded. One conflict is more than mechanical and is called out inline.
Relation to the existing reviews and to the complex
- @teknium1 / hermes-sweeper review 4677972075 (
review-verdict=keep_open salvageability=medium): its single blocker is resolved at this head. Not re-raised. - @abner-augusto (comment 5131474384) did the most valuable prior work here — a real rebase plus a live Guest Mode repro. I confirm both of his findings at code level and his structural diagnosis is correct:
telegram_guest_query_idis consulted in exactly one place in the entire tree (adapter.py:3583), with zero occurrences ingateway/run.py,gateway/session.pyorgateway/delivery.py. HisMessage_too_longfinding I therefore treat as already open and do not re-litigate; where my inline notes touch the same lines they add the parts he did not cover, and his question "which auxiliary path still sends?" is answered concretely inline. - @alt-glitch's duplicate classification against #32802 was retracted by its own author on 2026-07-30, correctly: our graph confirms #32802 CLOSED 2026-07-13, and #43049 / #51082 / #59879 / #68061 / #22263 all closed. #62551 is the only open PR in the complex (root issue #46196, still OPEN). It is the consolidation vehicle — @abner-augusto's framing of the actionable question is the right one.
- Our own earlier comment (5130675377) asked for a scoped diff and a rebase onto current main. Nothing here contradicts that; the inline notes are consistent with it.
- Complex neighbours worth a maintainer's eye: #527 (Gateway Permission Tiers — Owner/Admin/User/Guest) and #16017 (owner/guest/unknown/banned tiers) are both OPEN RFCs about precisely the user class this PR introduces. Combined with the
needs-decisionlabel already applied, the default-on question should be settled by a maintainer rather than by a dependency bump. TheP3 — Low, cosmetic, nice to havelabel looks hard to reconcile with a change to who can reach the agent, and the threesweeper:risk-*labels (session-state, message-delivery, compatibility) all fired accurately.
What is good and should survive the rework
Worth stating plainly, because the rework list is long: using the per-summon guest_query_id as the session lane is the right call — it gives key-level isolation from owner sessions for free. _numeric_message_thread_id is a clean fix with real tests. Extracting _register_handlers for testability was the right response to the earlier review. The compatibility fallbacks for missing PTB symbols are careful. Eleven focused tests for a feature of this size is above the bar for this queue.
(Note for the maintainer applying this: this reviewer cannot set a formal REQUEST_CHANGES state on this repository — please read the verdict above as the substantive one.)
3464527 to
9790f52
Compare
9790f52 to
cbdc06a
Compare
|
Rebased this PR onto current The current revision addresses the earlier review findings:
Fresh verification against the rebased range:
This branch remains the active end-to-end candidate for the still-unimplemented Guest Query path on |
cbdc06a to
5079108
Compare
|
Refreshed this branch onto current |
(cherry picked from commit 47a95dc)
Address blocking review comment. Moving the catch-all TypeHandler(Update) from group=-1 to fallback group=1 ensures it does not compete with normal text/command/media handlers in the default group (0). While PTB's group-dispatch rules evaluate groups independently (so a group=-1 catch-all does not prevent default group-0 handlers from running unless a handler raises ApplicationHandlerStop), this movement matches the placement in sibling PR NousResearch#32802 and keeps registration clean. Extracted adapter.py handler registration into _register_handlers() and added accompanying regression tests in test_telegram_guest_mode.py to assert both group-sorting and dispatch routing. (cherry picked from commit ae489fd)
Render final guest replies through the existing MarkdownV2 formatter before answerGuestQuery and cover headings, bold text, and GFM tables. (cherry picked from commit 0be553a)
(cherry picked from commit 32be344)
5079108 to
83a7eb8
Compare
|
Rebased and security-reviewed this branch onto current Blocker → fix checklist
Verification
@teknium1 @GottZ @abner-augusto — could you please re-review the refreshed head, especially the resolved trust-boundary and one-shot-delivery blockers? |
|
I tested the latest Test worktree:
Automated verification:
I also enabled The live Guest Mode tests succeeded:
The fixes appear to address the two runtime problems I previously reported:
I did notice two remaining UX concerns during the live test. 1. There is no visible processing indicationThe long Guest Query took approximately 53 seconds from intake to final delivery. During that time, the user receives no visible indication that the bot accepted the Guest Query and is working on it. The logs show the full gap clearly: I understand why ordinary interim messages and streaming cannot be used here: Guest Mode is a one-shot delivery path and must not consume the Could we check whether a non-message 2. Long answers are truncated rather than compactedThe final message displayed: This is working as a safety fallback, but the original answer was about 9,960 characters and the final Telegram message is bounded by Since Guest Mode cannot reliably send two separate messages with the same one-shot query, I would avoid splitting the answer into multiple A better UX would be to compact the answer before the final Guest Query delivery, keeping the direct answer, essential distinctions, important examples, and conclusion within a safe margin below Telegram's limit. The current hard truncation should remain as a final safety net, but ideally it should be exceptional rather than the normal behavior for long answers. For example, the Guest Mode finalization path could use a targeted instruction such as: Overall, the latest revision passed the requested automated and live validation, and the core Guest Mode delivery path now looks solid. My remaining recommendations are limited to the processing indication and pre-delivery compaction of long responses. |
|
Thank you for the thorough isolated automated and live validation. The 589 Telegram tests plus the changed-area checks, and especially the successful group and long private Guest Query runs, give useful confidence that the one-shot delivery and secrecy fixes are working as intended. I agree the two remaining points are UX opportunities rather than blockers for this fix:
I’m therefore keeping this PR scoped to the validated correctness and secrecy fixes and treating both recommendations as non-blocking follow-up design work. Thanks again for exercising the real Bot API path. |
Summary
python-telegram-botto 22.8 for Bot API Guest Query supportallow_guest_queriesopt-in that remains off by default and does not overload existingguest_modeguest_query_idupdatesanswerGuestQuerysendMessage; clarification returns control to the agent and approval-required operations are deniedsend_messagetool—including list/send/react/unreact—during Guest Query turns so it cannot bypassanswerGuestQuerySecurity model
Bot API Guest Queries can originate from chats where the bot is not a member, so support is disabled unless the operator explicitly enables
allow_guest_queries(orTELEGRAM_ALLOW_GUEST_QUERIES=true). Enabling it does not authorize new users: normal Telegram user authorization still runs, configuredallowed_chatsis enforced explicitly for PTB's specialsenderchat type, and topic/own-message/trigger gates remain active.Accepted Guest Queries otherwise use the same configured agent and tools as other authorized Telegram turns. They are session-isolated but are not a general reduced-permission tool sandbox. The cross-channel
send_messagetool is blocked to preserve the one-shot delivery boundary. The one-shot transport cannot host interactive clarification or approval buttons; operations requiring approval are denied automatically.Configuration
The rate limit is per sender/chat, clamped to 1–60 accepted queries per minute. State is bounded to prevent unbounded key growth.
Verification
Rebased onto current
main. The sole conflict was inpyproject.toml; resolution preserves upstreamaiohttp==3.14.3and this PR's PTB 22.8 requirement.tests/gateway/test_telegram*.pyUpdate.de_json()Guest Message intake probe: passedtest_platform_base.py: 84 passed, 1 skipped, 1 failed; the same credential-path test fails identically on untouched currentmainand is pre-existinguv lock --check: passed; PTB resolves to 22.8 and upstream aiohttp remains 3.14.3git diff --check: passedReview follow-up
This revision addresses the prior findings: handler ordering; explicit default-off trust-boundary gating; user/chat/topic scope; rate limiting and deduplication; long-answer formatting and bounded delivery; one-shot runner/session propagation; suppression of normal-send auxiliary paths; fail-closed approval/clarification handling; Guest media/location behavior; and lazy-dependency pin consistency.