feat(telegram): guest mode two-phase reply (Bot API 10.0, text-only) - #56476
feat(telegram): guest mode two-phase reply (Bot API 10.0, text-only)#56476elphamale wants to merge 7 commits into
Conversation
Related: supersedes the stale guest-mode reconstructions #51886 and #52639 (both still open), rebuilt fresh off |
|
Really like this one. The two-phase flow reads clean, and honestly the restart dedup, MEDIA stripping and busy-chat handling are better than what I had. I was writing my own guest-mode PR (#59879) and triage flagged it as a dup of this cluster, which is fair. So instead of competing I want to hand you the one bit I think is missing here. There's no auth check on who the guest caller is. Guest handling only checks allowed = self._telegram_allowed_chats()
if allowed and chat_id_str not in allowed:
return guest_mention # any @mention from any non-allowlisted chat passesFine as guest behavior, but it also means that with The fix is small and stays inside the adapter (no gateway changes). Gate the caller in # after parsing `msg`, before registering guest state
caller = getattr(msg, "guest_bot_caller_user", None) or msg.from_user
caller_id = str(getattr(caller, "id", "") or "").strip()
if not self._is_guest_caller_authorized(caller_id): # empty allowlist => deny all
logger.warning("[%s] Unauthorized guest caller: %s", self.name, caller_id)
return
One thing I'm curious about: state is keyed by |
|
Thanks for this — and for closing #59879 to hand it over instead of racing it. Genuinely good sportsmanship, appreciated. I like both ideas and I'm going to do them. Caller auth (fail-closed): you're right, this is the gap. On a bot wired to an LLM + tools, "anyone who knows the @handle can drive it from any group" is an open door, and the approval work elsewhere in this stack only gates dangerous commands — it does nothing about someone just burning tokens or slipping in a prompt injection. Hermes is aimed at individuals / small teams, so the right default is a tight allowlist, not gen-pop access, with Session bleed: this one's a nice catch. Funny timing — I'd been going back and forth on exactly this and had talked myself out of the obvious version: my first instinct was a brand-new session per guest turn, but I killed it because a chatty user would spawn a pile of throwaway sessions. Your per-turn suffix is the same instinct, and it made me realize the middle key I should've reached for is Will ping you when these land. Thanks again. |
|
Both landed on this branch, co-authored to you. Caller gate — fail-closed in Session bleed — your suffix idea pushed me to a slightly smaller version. Since the caller gate means guests are now authorized users, I just stamp the real caller id onto the event source and let the existing group keying isolate them: Thanks again — this was a good catch and a clean handoff. |
ce6d48d to
353c7f2
Compare
Two guest turns from the same chat landed on one shared, chat-keyed session, so a previous caller's context bled into the next caller's turn. build_session_key already isolates group participants by user_id, but a guest message carries no from_user, so _build_message_event left user_id unset and the group key collapsed to one session per chat. Stamp the real caller id (from guest_bot_caller_user, captured at the auth gate) onto the event source. Post-gate the caller is an authorized user, so this needs no guest-specific isolation branch in session.py — it just gives the existing group keying the id it was missing, making guest sessions key exactly like ordinary group sessions (per-caller when group_sessions_per_user is on, the default). Simpler than a bespoke session-key suffix and behaves consistently with non-guest group traffic. Finding by mazzz3r on NousResearch#56476 (session context bleed between guest callers). Co-authored-by: mazzz3r <mazzz3r@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018H2eDAi2CWiHig1dd65Zg4
3479a9f to
946e4e7
Compare
8da4ed4 to
34d2dfb
Compare
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Four PRs address or reference the missing Telegram Bot API 10.0 guest-message delivery path. #51886 and #52639 are stale cumulative prototypes, while #56476 cleanly rebuilds the text-only guest-reply foundation and #56477 adds the stacked media-delivery flow.
Related pull requests
- #51886 [closed]
duplicate— (+1406/-379) — superseded prototype: Implements guest_message ingestion, answerGuestQuery stubs, buffered/edited text replies, and media handling, but the diff also carries substantial unrelated Telegram changes and a personalized thinking-verbs file; it remains relevant as the original implementation now cleanly rebuilt as #56476 and #56477. - #52639 [closed]
related— (+1381/-217) — superseded cumulative media prototype: Extends the two-phase guest reply with lazy stubs and native media staging, but includes the stale #51886 foundation plus unrelated changes; its text foundation is replaced by #56476 and its media scope by #56477. - #56476
related— (+953/-5) — canonical text foundation: Adds explicit guest_message allowed-updates and handling, fail-closed caller authorization, per-caller session isolation, in-flight-query protection, restart deduplication, buffered answerGuestQuery/editMessageText delivery, and focused regressions. This is consistent with the keep_open review on #56476; its blocking UTF-16 truncation finding is addressed in the diff with UTF-16-aware truncation and an emoji-boundary test. - #56477
related— (+1711/-8) — stacked media follow-up: Builds on #56476 with contained media staging, short-lived deliver_ redemption, authorized direct dispatch, stale-file avoidance, and final-segment-only buffering. This is consistent with the keep_open review on #56477; the missing path-translation call and insufficient handler coverage identified there were replaced with an existing cache-path translator and real-handler tests.
Duplicates
#51886 substantially overlaps the #56476 text foundation and parts of #56477; #52639 cumulatively overlaps both #56476 and #56477. #56477 is not a duplicate of #56476 because it is the intentionally stacked media extension.
Suggested consolidation
Merge #56476 as the canonical text-only guest-mode foundation, then restack and evaluate #56477 as its media follow-up. #51886 and #52639 can remain closed as superseded duplicates of the clean #56476/#56477 split.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup51886 ["PRs duplicating each other"]
P51886["PR #51886 (closed)"]
P56476["PR #56476 (open)"]
end
class P51886 closed
class P56476 open
class P56476 target
click P51886 "https://github.com/NousResearch/hermes-agent/pull/51886"
click P56476 "https://github.com/NousResearch/hermes-agent/pull/56476"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 380 kB of PR diffs, 17 kB of issue/PR text, 21 kB of discussion (17 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
34d2dfb to
4244de8
Compare
Handles guest_message updates for chats the bot is not a member of: fires a text "thinking" stub via answerGuestQuery immediately, buffers the streamed reply, then edits the stub in place via editMessageText using the returned inline_message_id once the response is ready. Text-only foundation — no media delivery yet, and no discernment of query content; the stub always fires and the platform layer does no classification. Media delivery lands in a follow-up PR.
- Reject a second concurrent guest query for a chat already mid-turn instead of overwriting _pending_guest_queries/_guest_inline_message_ids/ _guest_reply_buffer, which orphaned the first stub and let the two replies' buffered text cross-contaminate. - Factor the repeated "is this chat_id a guest chat" check (send, send_status_message, edit_message_draft) into _is_guest_chat(). - Trim the in-memory seen-update-id set to 200 to match the on-disk persisted cap (was 500 in memory vs 200 on disk).
Guest mode (@mention from a chat the bot isn't a member of) previously gated only the chat/mention via _should_process_message, never the person — so with guest_mode: true, anyone who knew the bot's @handle could drive the full LLM + tools from any group (cost, abuse, prompt injection). The approval work elsewhere only stops dangerous *commands*; it does nothing about this entry point. Gate the caller in _handle_guest_message_update, fail-closed, before any state registration or the deliver_<token> branch (so token redemption is gated too). The human caller for a guest update is carried in the raw guest_bot_caller_user field, NOT from_user (absent/unreliable here); PTB's de_json drops the field it doesn't model, so it's read from the raw payload. Authorization routes through the same _is_callback_user_authorized the exec-approval buttons use — one definition of "who's allowed" (env allowlists + pairing store, with * as the explicit open-to-all opt-out). Empty allowlist or unknown caller => deny. A missing caller field logs the payload keys loudly so a field-name drift is diagnosable rather than a silent deny-all. Co-authored-by: mazzz3r <mazzz3r@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018H2eDAi2CWiHig1dd65Zg4
Two guest turns from the same chat landed on one shared, chat-keyed session, so a previous caller's context bled into the next caller's turn. build_session_key already isolates group participants by user_id, but a guest message carries no from_user, so _build_message_event left user_id unset and the group key collapsed to one session per chat. Stamp the real caller id (from guest_bot_caller_user, captured at the auth gate) onto the event source. Post-gate the caller is an authorized user, so this needs no guest-specific isolation branch in session.py — it just gives the existing group keying the id it was missing, making guest sessions key exactly like ordinary group sessions (per-caller when group_sessions_per_user is on, the default). Simpler than a bespoke session-key suffix and behaves consistently with non-guest group traffic. Finding by mazzz3r on NousResearch#56476 (session context bleed between guest callers). Co-authored-by: mazzz3r <mazzz3r@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018H2eDAi2CWiHig1dd65Zg4
…length hermes-sweeper flagged a blocking bug in the guest two-phase reply flush: _plain[:4096] slices by Python string length, but Telegram's 4,096 limit is UTF-16 code units — e.g. 2,049 emoji is 2,049 chars but 4,098 units. A heavy-emoji final reply could pass this truncation and then fail the real editMessageText call, silently dropping the turn after guest state had already been torn down. Reuses the existing _truncate_stream_overflow_preview() helper (same UTF-16-aware truncate_message() the regular streaming edit path already uses) instead of reinventing truncation. Also switched the typewriter animation to iterate over the already-truncated _reply_text instead of raw _plain — it was typing out untruncated content and then visibly "shrinking" to the truncated text on the final edit, on top of risking oversized intermediate frames for the same UTF-16 reason. Also drops the dead thinking_verbs.py dynamic exec loader (the file doesn't exist in this PR or on main, so the broad except always fell through to the same ["Thinking"] fallback) per sweeper's suggestion — replaced with the static list it always resolved to anyway. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
The previous commit removed the exec-based thinking_verbs.py loader as dead code, since the file didn't exist anywhere in this PR or on main — but that was the wrong fix. The file existed in this same feature's earlier iteration (NousResearch#51886, "feat(telegram): add bundled thinking_verbs.py with minimal curated list") and was simply dropped when this branch was rebuilt fresh off main. The loader itself is intentional: it lets operators customize the stub's progress-verb list (translate it, curate it to match SOUL.md persona, swap in emojis) via ~/.hermes/local-patches/plugins/platforms/telegram/thinking_verbs.py without needing a code change, matching this codebase's established local-patches override pattern. Restored the file from NousResearch#51886 (Thinking/Pondering/Cooking/Toolcalling/ Yarning) and the original try/except exec loader. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
check-windows-footguns.py flagged open() without encoding= -- platform-default encoding (cp1252/mbcs on Windows) can mojibake this file's contents. Rebased onto the fixed feat/telegram-inline-toolside (NousResearch#52683) as part of today's PR-conflict drift audit -- both PRs insert a handler registration at the same point in TelegramAdapter.connect(); purely textual, kept both. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
4244de8 to
ce69ddb
Compare
sweeper found that the guest-mode fail-closed check only lived inside _await_gateway_decision, reached only after smart approval (approvals.mode: smart, the default) or a cached per-session grant (is_approved) already returned approved=True -- both silently bypassed the guest denial. Move the guest check earlier in check_all_command_guards (right after computing tirith/dangerous-command findings, before cached grants are consulted or smart approval runs) and add the equivalent early check to check_execute_code_guard. Factor the shared BLOCKED result into _guest_unsupported_block_result() so all four guest-denial sites (two new early checks, two existing _await_gateway_decision branches) return the identical message/shape. Also extracted gateway/run.py's inline _is_guest_chat getattr wiring into _resolve_approval_session_is_guest(), so it's testable against the concrete Bot API 10.0 guest predicate from NousResearch#56476 (same _pending_guest_queries / _guest_only_chats shape) rather than only through mark_session_guest's internal state directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
Summary
guest_messageupdates for chats the bot is not a member ofanswerGuestQueryimmediately on first activity, buffers the streamed reply, then edits the stub in place viaeditMessageText(using the returnedinline_message_id) once the response is readymainfrom the live, production-tested guest-mode code, superseding the earlier stale reconstructions in feat(telegram): two-phase guest reply — immediate stub + progressive streaming #51886 and feat(telegram): guest-mode native media delivery + lazy two-phase stub #52639 (to be closed with a pointer to this PR)Test plan
tests/gateway/test_telegram_guest_reply.py— stub-fire and OPC text-flush coverage_make_adapter()helpers intest_telegram_reactions.py,test_telegram_thread_fallback.py,test_telegram_username_chat_id.pyto include the new guest-mode instance attributes (these tests construct the adapter viaobject.__new__, bypassing__init__)tests/ -k telegramsuite passes with no new regressions vs. a cleanmainbaselineUpdate: UTF-16 truncation fix (addresses hermes-sweeper review)
hermes-sweeper flagged a blocking bug: the final OPC flush truncated the reply with
_plain[:4096]— Python string length, not UTF-16 code units. Telegram's 4,096 limit is UTF-16 units, so e.g. 2,049 emoji is 2,049 chars but 4,098 units — a heavy-emoji reply could pass this truncation and then fail the realeditMessageTextcall, silently dropping the turn after guest state had already been torn down.Fixed by reusing the existing
_truncate_stream_overflow_preview()helper (the same UTF-16-awaretruncate_message()the regular streaming edit path already uses) instead of a naive slice. Also switched the typewriter animation to iterate over the already-truncated text instead of the raw buffered reply — it was typing out untruncated content and then visibly "shrinking" on the final edit, on top of risking oversized intermediate frames for the same reason. Added a regression test (test_branch1_opc_truncates_by_utf16_units_not_python_length) that asserts everyeditMessageTextcall — typewriter frames and the final edit — stays within the UTF-16 cap.Sweeper also flagged the
thinking_verbs.pydynamic-exec loader as dead initialization, since the file doesn't exist in this PR or onmain. That diagnosis was right but my first fix (dropping the loader for a hardcoded["Thinking"]) was wrong — the file existed in this same feature's earlier iteration, #51886 ("feat(telegram): add bundled thinking_verbs.py with minimal curated list"), and was simply dropped when this branch was rebuilt fresh offmain. Restored it instead:The loader itself is intentional, not dead code: it's designed so operators can customize the stub's progress-verb list — translate it, curate it to their bot's persona, swap in emojis — by dropping a replacement file at
~/.hermes/local-patches/plugins/platforms/telegram/thinking_verbs.py, without needing a code change. That's the same override mechanism this codebase already uses elsewhere for personal/operator-specific customization. The bug was that the bundled default got lost in the rebuild, not that the loader mechanism was wrong.🤖 Generated with Claude Code