Skip to content

fix(telegram): observe-mode slash commands act on the shared group session - #131336

Open
jonpol01 wants to merge 1 commit into
NousResearch:mainfrom
jonpol01:fix/telegram-observe-command-session
Open

jonpol01 wants to merge 1 commit into
NousResearch:mainfrom
jonpol01:fix/telegram-observe-command-session

Conversation

@jonpol01

@jonpol01 jonpol01 commented Oct 2, 2026

Copy link
Copy Markdown

What does this PR do?

In a Telegram observe chat (observe_unmentioned_group_messages), text turns and observed chatter use a sender-less source and share one group session. Slash commands keep the sender's user_id on purpose so slash access checks can identify the sender (#67816), but with the default group_sessions_per_user that same id keyed them to a per-user session. /new reset an empty session, /model / /undo / /compress acted on the wrong one, and skill commands ran without the observed context.

The sender id cannot simply be dropped from commands (it is the authorization input), so the session's sharing is declared separately. SessionSource.shared_session is set by the Telegram adapter for every observe-chat event (text, observed chatter, commands). build_session_key does not add the participant when it is set, and is_shared_multi_user_session returns True, so the key and the guards that mirror it (_is_shared_session_source) stay in lock-step. Commands keep user_id for _check_slash_access / _is_user_authorized, unchanged.

Effects beyond the key, all intended:

  • Command turns render the same session-context prompt as text turns (multi-user session line, no pinned user name), so a skill command no longer flips the prompt against the conversation's text turns.
  • A command that runs an agent turn gets the usual [sender] prefix of shared sessions; observe text turns already carry [nickname|user_id] and have no user_name, so they gain no second prefix.
  • Existing observe sessions render the multi-user session line from their next turn on: one re-render, once.

shared_session round-trips through to_dict / from_dict, written only when set, so existing stored origins stay byte-identical. An event rebuilt from a stored origin (internal wakes, completions) keys the same shared session even when the last turn was a command. A peer that could set it could already send user_id: null for the same key, so it grants nothing new.

Related Issue

Fixes #131335

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/session.py: SessionSource.shared_session (serialized only when set); build_session_key and is_shared_multi_user_session honour it.
  • plugins/platforms/telegram/adapter.py: _telegram_group_observe_shared_source sets it; _apply_telegram_group_observe_attribution sets it on commands while keeping their sender.
  • tests/gateway/test_telegram_group_gating.py: one invariant test through the real _handle_text_message / _handle_command. The addressed text turn, the command, the observed-chatter source and the command source restored through to_dict / from_dict all build one session key; the command still carries the sender id; command and text turns render the same session-context prompt.
  • Docs: website/docs/user-guide/messaging/telegram.md and its zh-Hans mirror now say slash commands in an observe chat act on the shared session while admin-only commands are still checked against the sender.

How to Test

scripts/run_tests.sh tests/gateway/test_telegram_group_gating.py

  • Red on main: AssertionError: {'agent:main:telegram:group:-100', 'agent:main:telegram:group:-100:111'}.
  • Green with the fix.
  • Sabotage: not serializing the flag fails the restored-origin key; not honouring it in is_shared_multi_user_session fails the prompt parity.
  • Related: all tests/gateway/test_telegram_*.py, tests/plugins/test_telegram_update_admission.py, test_session.py, test_prompt_tail_freeze.py, the slash-session tests (77 files): 735 passed, 3 skipped. Every other test file using build_session_key / SessionSource.from_dict / is_shared_multi_user_session (60 files): 418 passed, 2 failed, both unrelated. test_kanban_wake_acceptance.py::test_push_receipt_requires_real_admission_without_displacing_user[True] fails identically on main in this env (python-telegram-bot not installed); test_completion_admission.py::test_unavailable_raw_route_is_quiet_without_hiding_invalid_routes timed out a 5 s WAL scan under 10 workers and passes alone. Ruff clean; check_doc_links.py passes. Python 3.14.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass. I ran only the related files listed above via scripts/run_tests.sh, not the full suite.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…ssion

In an observe chat, text turns and observed chatter use a sender-less
source, so they share one group session. Slash commands keep the
sender's user_id for slash access checks (NousResearch#67816), and with the default
group_sessions_per_user that id keyed them to a per-user session:
/new, /model, /undo and skill commands acted on a session the observed
transcript and the agent's history never reached.

SessionSource gains shared_session, set by the Telegram adapter for
observe-chat events. build_session_key and is_shared_multi_user_session
honour it, so commands keep their sender for authorization but share the
group session and render the same session-context prompt as text turns.
It round-trips through to_dict/from_dict (written only when set) so
events rebuilt from a stored origin key the same session.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Oct 2, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

The core fix is right, and the regression test is a real one — I mutated all six production predicates this PR touches (build_session_key's isolate_user branch, is_shared_multi_user_session's short-circuit, to_dict, from_dict, and both adapter branches) and every one turned the suite red, including reverting the command branch to its pre-fix form. Forum-topic sessions with thread_sessions_per_user on either value hold the same key as the observed transcript.

One regression from the flag landing on the command source.

plugins/platforms/telegram/adapter.py:6269 sets shared_session=True on the command source, which keeps user_name (correctly — slash access needs the sender). But gateway/run_inbound.py:1465 prefixes on if _is_shared_multi_user and source.user_name:, and both are now true for a command, so run_inbound.py:1474 rewrites the body to f"[{_safe_user_name}] {message_text}".

Skill and bundle slash commands are rewritten into a natural-language prompt by _hm_skill_slash_rewrite (gateway/run_inbound.py:1250) and then fall through to the agent turn — that is exactly the text the model is told to obey. Driving the real adapter branch into the real runner prefix:

base: 'Use the summarise skill on the transcript above.'
head: '[Alice] Use the summarise skill on the transcript above.'

neutralize_untrusted_inline_text already strips control characters, so this is not an injection hole — but the command body is now attributed as something a participant said in the group, which is the opposite of what a skill invocation is. Observed text turns are unaffected because that branch nulls user_name.

Cheapest fix is excluding commands at the prefix. Worth considering whether the flag belongs on SessionSource at all: to_dict now persists it, so a shared command session restores as shared, and the command turn's own system prompt flips to the "Multi-user session" form (gateway/session.py:424), which changes the cached prefix mid-conversation. Scoping the shared key to command dispatch without a source field would avoid both.

Also checked and clean: the cross-origin /resume and /sessions guards widening alongside the key is correct — the session really is shared now, so requiring a matching participant would contradict build_session_key.

@jonpol01

jonpol01 commented Oct 2, 2026

Copy link
Copy Markdown
Author

Thanks for the mutation pass. I checked both points on the PR head (dbba06d). No code change from this round.

1. [user_name] prefix on skill/bundle commands. The text does change from base, but the new text is what every shared session already does. _prefix_inbound_sender_context (gateway/run_inbound.py:1459) is not touched by this PR. It attributes the trigger turn in any session where is_shared_multi_user_session is true, and command turns are included. I drove it with a skill-rewritten COMMAND event on existing shared sessions:

telegram group, group_sessions_per_user=False: '[Alice] Use the summarise skill on the transcript above.'
telegram thread (shared by default):           '[Alice] Use the summarise skill on the transcript above.'
discord  group / thread:                       '[Alice] Use the summarise skill on the transcript above.'
slack    group / thread:                       '[Alice | Slack user <@111>] Use the summarise skill ...'

In observe mode, the triggered text turn in the same session is already attributed by the adapter ([Alice|111]\n..., _telegram_group_observe_attributed_text). On base, the command turn was the only unattributed turn, and that was only because it was keyed to a separate per-user session. The prefix was added to keep sender attribution in shared group sessions (04f9ffb). Leaving commands out at the prefix would change that for every platform's shared sessions, and the model would lose track of who invoked the skill in a multi-user transcript. The name is still passed through neutralize_untrusted_inline_text, as you noted. So I'm keeping it as is.

2. shared_session on SessionSource / persisted / system prompt. Persisting it is deliberate. A restored command source has to resolve to the same shared key, otherwise a gateway restart would split the session again. The regression test asserts the round-trip. I checked the prompt in both versions, and the flip happens on base, not on the head:

text key      agent:main:telegram:group:-100
cmd base key  agent:main:telegram:group:-100:111
cmd head key  agent:main:telegram:group:-100
base cmd prompt == text prompt: False
head cmd prompt == text prompt: True
restored shared: True

With the fix, every turn in the shared observe session renders the same "Multi-user session" context, so the cached prefix stays byte-stable. The test already pins this (build_session_context_prompt(...) equality between the command and text turns). A key that exists only during command dispatch, with no source field, would lose the flag on restore and on any path that rebuilds the key from the stored source (/resume, /sessions, eviction).

Docs: N/A. Nothing changed in this round.

samuelcardillo added a commit to samuelcardillo/hermes-agent that referenced this pull request Oct 7, 2026
…ode parity)

Rebase + rework of NousResearch#133418 addressing the review findings (F-001..F-004):
NousResearch#133418 (review)

Core design change (F-001, F-003): shared-session routing is now declared on the
SessionSource (``shared_session: bool``) instead of erasing the caller identity.

- Attributed text turns, commands and observed chatter KEEP ``user_id``, so
  ``_hm_admit_event`` authorizes an already-allowed participant exactly as before —
  no second chat-grant configuration is required for the feature to work, and
  observation permission never silently becomes invocation permission.
- ``build_session_key`` / ``is_shared_multi_user_session`` honor ``shared_session``,
  so observed chatter, addressed turns, commands and origins restored through
  ``to_dict``/``from_dict`` all resolve to ONE shared group lane. ``/new`` and other
  session-scoped commands now act on the conversation the bot is actually in, while
  slash-access sender checks keep working. The field round-trips only when set, so
  existing stored origins stay byte-identical. (Same field name/contract as NousResearch#131336
  so the two PRs compose.)

F-002: a single effective observe scope (``_whatsapp_observe_scope_active``) governs
BOTH collection and attribution: feature flag + explicit ``observe_allowed_chats`` +
mention-gated intake (require_mention on, chat not free-response). With
``require_mention: false`` a message is processed once, normally, and never observed;
free-response chats keep their original principal and text.

F-004: the observer emits a ``datetime`` object, which ``coerce_epoch`` accepts —
verified by a real SessionStore/SessionDB round-trip with exact epoch readback and no
corrupt-timestamp warning.

The WHATSAPP_GROUP_ALLOWED_CHATS authz registration is retained as a general
capability (mirrors Telegram's TELEGram_GROUP_ALLOWED_CHATS for user-less group
principals) but is no longer required by this feature.

Tests (18): observe gating matrix, effective-scope exclusivity
(require_mention off / free-response), principal-preserving shared routing with the
text/command/observed/restored one-key invariant, real-database timestamp round-trip,
history-builder marker recognition. Sibling suites: 118 passed.

Driver: live WhatsApp bot validated (unmentioned chatter observed and injected on the
next mention; mention-only reply behavior preserved).

This branch has not been deployed

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists platform/telegram Telegram bot adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Telegram observe mode: slash commands in non-forum groups hit a per-user session instead of the shared observed session (/new, /model, skills)

3 participants