Skip to content

refactor(telegram): extract authorization cluster into TelegramAuthorizationMixin - #75742

Open
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:refactor/telegram-authz-mixin
Open

refactor(telegram): extract authorization cluster into TelegramAuthorizationMixin#75742
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:refactor/telegram-authz-mixin

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

Closes #75741.

TelegramAdapter is 9,184 lines and 193 methods, the largest platform adapter in the tree. This lifts its authorization cluster into plugins/platforms/telegram/authz_mixin.py, the same mechanical extraction that produced gateway/authz_mixin.py for GatewayRunner, and the Telegram counterpart to #75735.

adapter.py goes 10,019 -> 9,747 lines. The class goes 9,184 -> 8,912 lines and 193 -> 184 methods.

What moved

263 lines, verbatim, from two regions about 6,800 lines apart:

callable was at role
_is_callback_user_authorized 908 callback-query gate
_source_from_message_for_auth 960 resolves the session source the gate keys on
_telegram_auth_env_configured 1020 whether any allowlist is set at all
_is_user_authorized_from_message 1032 the main gate
_telegram_allowed_chats 7757 chat scope
_telegram_group_allowed_chats 7772 group scope
_telegram_observe_allowed_chats 7781 observe scope
_telegram_allowed_topics 7796 topic scope
_telegram_ignored_threads 7811 thread denylist

Where the boundary is, and why

The five scope readers sit in a run of ten similar-looking config readers. The other five did not move, deliberately.

_telegram_guest_mode, _telegram_exclusive_bot_mentions, _telegram_free_response_chats, _telegram_free_response_topics and _telegram_is_free_response_topic answer "should the bot reply to this", which is routing policy. The five that moved answer "is this permitted", which is a trust decision. They read alike and live side by side, but only one group belongs behind a name ending in AuthorizationMixin, and mixing them would make the module's name stop meaning anything precise.

_coerce_allow_set was imported into the adapter for the sole use of _is_user_authorized_from_message, so it travels with it; the adapter's import of it is replaced by the mixin import rather than left dangling.

Why it is safe

1. The bodies are byte-identical. A script parses the pre-refactor adapter.py out of git and the two post-refactor files, extracts each callable's exact source span, and compares strings. All 9 match. The same script confirms exactly those 9 methods left TelegramAdapter, that nothing was added, and that no other method in the class changed by a single character.

2. Method resolution is unchanged. TelegramAuthorizationMixin precedes BasePlatformAdapter, giving TelegramAdapter -> TelegramAuthorizationMixin -> BasePlatformAdapter -> ABC. Every method name defined on the old class still resolves, and all 9 resolve to the mixin.

3. Module-level names still resolve to the same objects.

  • logger is bound as logging.getLogger("plugins.platforms.telegram.adapter") rather than __name__, so records emitted from the moved methods keep the name they had. getLogger is a registry lookup, so this is the adapter's own logger object. Verified at runtime: authz_mixin.logger is adapter.logger.
  • Message is imported under the adapter's own ImportError guard with the same Any fallback. Verified at runtime: authz_mixin.Message is adapter.Message.

Unlike plugins/platforms/discord/adapter.py, this module does not enable postponed annotation evaluation, so Message in a lifted signature is evaluated when the function is defined rather than left as a string. The new module deliberately omits from __future__ import annotations for that reason: adding it would have silently turned the lifted annotations into strings. Verified by reading __annotations__ off the composed class after the move.

No import cycle: authz_mixin imports nothing from adapter.

Verification

Differential run against clean upstream/main at the same base commit, in two isolated worktrees, over tests/gateway (53 Telegram test files), tests/plugins and tests/tools:

clean main with this change
passed 10,459 10,459
failed 88 88
skipped 79 79

The failures are identical in both runs and pre-existing on main. Set difference in both directions is empty: nothing new fails, and nothing that failed before now passes.

No test file is modified by this PR.

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/plugins Plugin system and bundled plugins platform/telegram Telegram bot adapter area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 1, 2026
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the focused mechanical extraction. Current origin/main is the PR base (e444d165), and it still contains this authorization cluster in plugins/platforms/telegram/adapter.py:9081103 and :77577830. The PR moves those methods into plugins/platforms/telegram/authz_mixin.py:48 and changes the adapter bases to put the mixin before BasePlatformAdapter, preserving the intended lookup order. This matches the established Gateway authorization-mixin design (gateway/authz_mixin.py:61).

No concrete correctness, completeness, or design-fit problem was found in the reviewed diff.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 1, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two PRs address the same god-file decomposition pattern on separate platform adapters: #75735 extracts Discord authorization from four dispersed regions, while #75742 extracts Telegram authorization and scope readers from two dispersed regions. Both diffs target the reported auditability problem through platform-specific mixins while preserving mixin-first method resolution order.

Related pull requests

  • refactor(discord): extract authorization cluster into DiscordAuthorizationMixin #75735 best fix — (+553/-503) — keep open with a salvage path: the diff consolidates the Discord authorization cluster into DiscordAuthorizationMixin, but the contributor keep_open review identifies two concrete namespace regressions: the absolute import can initialize a second adapter package under directory-plugin loading, and the hard-coded logger does not preserve the active namespaced adapter identity. Retain the extraction, change it to a relative mixin import, derive the logger from the active package, and add the requested namespaced PluginManager-load regression test.
  • refactor(telegram): extract authorization cluster into TelegramAuthorizationMixin #75742 best fix — (+323/-274) — keep open with a salvage path: the diff moves the Telegram authorization gates and scope readers into TelegramAuthorizationMixin while leaving routing-policy helpers on the adapter, directly matching the reported boundary. Consistent with the maintainer-bot keep_open verdict, the valuable salvage path is the focused mechanical extraction and preserved mixin-before-BasePlatformAdapter lookup order; its cross-PR relationship is architectural rather than duplicative.

Suggested consolidation

Keep both #75735 and #75742 open with platform-specific salvage paths; they implement parallel Discord and Telegram cuts rather than duplicate changes. #75735 requires the contributor-identified relative-import, active-package logger, and namespaced-loader test corrections, while #75742 should remain the separate Telegram extraction in line with its maintainer-bot keep_open verdict; neither PR can be closed as a duplicate of the other.

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
    I75741(["issue #75741 (open)"])
    P75742["PR #75742 (open)"]
    P75742 -->|best fix| I75741
    class I75741 open
    class P75742 open
    class P75742 best
    class P75742 target
    click I75741 "https://github.com/NousResearch/hermes-agent/issues/75741"
    click P75742 "https://github.com/NousResearch/hermes-agent/pull/75742"
Loading

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 (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 81 kB of PR diffs, 13 kB of issue/PR text, 2 kB of discussion (3 comments), 6 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@andrexibiza

Copy link
Copy Markdown
Contributor

Coordination on the allowlist-getter overlap (cross-PR composition, from the adapter sharding wave, epic #78791):

Your authz_mixin.py defines 5 allowlist getters (, , , , ) that also exist in the config/mention shard (TelegramConfigMentionMixin, wave-2 shard A5 — mixin is FIRST base of TelegramAdapter).

Verified 2026-08-04:

  • A5's versions are CURRENT: they use (13 uses — the profile-scoped env pattern from the main codebase).
  • This PR's versions are STALE: they use (8 uses — the older pattern).
  • MRO outcome: A5's mixin is first in 's base chain, so A5's implementations win at runtime regardless of merge order.

Recommendation: when rebasing this PR, DROP the 5 duplicate getters from authz_mixin.py (keep the genuine authz methods — , , , etc.) and let them resolve via A5's mixin. Credit: the shard wave preserved the current semantics deliberately — @MaxFreedomPollard's authz slice is otherwise welcome, the duplication is the only overlap.

@andrexibiza

Copy link
Copy Markdown
Contributor

Coordination on the allowlist-getter overlap (cross-PR composition, from the adapter sharding wave, epic #78791):

Your authz_mixin.py defines 5 allowlist getters (_telegram_allowed_chats, _telegram_group_allowed_chats, _telegram_observe_allowed_chats, _telegram_allowed_topics, _telegram_ignored_threads) that also exist in the config/mention shard (TelegramConfigMentionMixin, wave-2 shard A5 — mixin is FIRST base of TelegramAdapter).

Verified 2026-08-04:

  • A5's versions are CURRENT: they use _scoped_gate_env (13 uses — the profile-scoped env pattern from the main codebase).
  • This PR's versions are STALE: they use os.getenv (8 uses — the older pattern).
  • MRO outcome: A5's mixin is first in TelegramAdapter's base chain, so A5's implementations win at runtime regardless of merge order.

Recommendation: when rebasing this PR, DROP the 5 duplicate getters from authz_mixin.py (keep the genuine authz methods — _is_callback_user_authorized, _telegram_auth_env_configured, _should_pass_unauthorized_dm_for_pairing, _is_user_authorized_from_message etc.) and let them resolve via A5's mixin. Credit: the shard wave preserved the current _scoped_gate_env semantics deliberately — @MaxFreedomPollard's authz slice is otherwise welcome; the duplication is the only overlap.

@andrexibiza

Copy link
Copy Markdown
Contributor

Re: the auth-cluster overlap with the adapter sharding wave (epic #78791) — verified against pr-75742-head @ f05870c1 on 2026-08-04:

  1. The mixin is not yet behavior-neutral. _is_user_authorized_from_message in authz_mixin.py is a rewritten early-return variant that deletes the pairing fallthrough (_should_pass_unauthorized_dm_for_pairing is gone entirely, and the tail return self._should_pass_unauthorized_dm_for_pairing(source) is dropped). That breaks the gateway pairing handshake for unauthorized/allowlisted DMs when unauthorized_dm_behavior resolves to pair — covered by test_allowlist_dm_with_explicit_pair_behavior_reaches_gateway, test_allow_from_dm_with_pair_override_reaches_gateway, and test_unauthorized_dm_with_pair_behavior_builds_event in tests/gateway/test_telegram_auth_check.py. Please restore the original accumulate-and-fallthrough flow verbatim.

  2. os.getenv vs _scoped_gate_env. _is_callback_user_authorized, _telegram_auth_env_configured, and the final fallback read os.getenv — under gateway.multiplex_profiles that can return another profile's allowlist ([Bug]: Discord adapter allow/deny gates are process-global, breaking per-profile isolation under multiplex_profiles #72348 Telegram mirror). The adapter's current code uses _scoped_gate_env (gateway.authz_mixin._platform_gate_env); please switch (lazy-import _scoped_gate_env from the adapter module as the config shard does).

  3. Drop the 5 allowlist getters (_telegram_allowed_chats, _telegram_group_allowed_chats, _telegram_observe_allowed_chats, _telegram_allowed_topics, _telegram_ignored_threads) from the mixin per the earlier coordination: the wave-2 TelegramConfigMentionMixin (first base of the sharded TelegramAdapter) already defines them with the current _scoped_gate_env semantics and wins at runtime regardless of merge order — the copies here are stale duplicates.

Once those three land, the shard wave will delete the adapter's residual 5-method auth cluster and inherit through TelegramAuthorizationMixin in the MRO (proposed end-state class line in the lane-3 plan). Happy to take the restore patch if useful.

…izationMixin (god-file Phase 3)

TelegramAdapter is 9,184 lines and 193 methods, the largest platform adapter
in the tree. This lifts its authorization cluster to
plugins/platforms/telegram/authz_mixin.py, the same mechanical mixin
extraction that produced gateway/authz_mixin.py.

Moved verbatim (263 lines):

  the authorization decision (908-1103)
    _is_callback_user_authorized      _telegram_auth_env_configured
    _source_from_message_for_auth     _is_user_authorized_from_message

  the chat / topic / thread scope readers (7757-7830)
    _telegram_allowed_chats           _telegram_allowed_topics
    _telegram_group_allowed_chats     _telegram_ignored_threads
    _telegram_observe_allowed_chats

The boundary is deliberate: what moves answers 'is this permitted'. The
mention, guest-mode and free-response helpers answer 'should the bot reply'
and stay on the adapter, because they are routing policy rather than a trust
decision.

Behavior-neutral. Every body is byte-identical to what it replaced; the only
edits are the class declaration, one swapped import, and the new module
header. The mixin precedes BasePlatformAdapter in the bases so resolution
order is unchanged. logger is bound by explicit name, so records keep the name
plugins.platforms.telegram.adapter and getLogger returns the adapter's own
object. Message is imported under the adapter's own ImportError guard, and
this module deliberately does not enable postponed annotation evaluation,
matching the adapter, so the lifted signatures evaluate exactly as before.

_coerce_allow_set was imported for the sole use of
_is_user_authorized_from_message and moves with it, so the adapter's import of
it is replaced by the mixin import.

adapter.py 10,019 -> 9,747 lines; the class 9,184 -> 8,912 lines and 193 -> 184 methods.
@MaxFreedomPollard
MaxFreedomPollard force-pushed the refactor/telegram-authz-mixin branch from f05870c to b5b8154 Compare August 13, 2026 21:13
@MaxFreedomPollard

Copy link
Copy Markdown
Contributor Author

@andrexibiza thank you for digging into this properly, both findings were correct and the first one was worse than it looked. Rebased onto current main at b5b8154.

The pairing fallthrough. You were right that _is_user_authorized_from_message had been rewritten into an early-return variant that dropped the tail return self._should_pass_unauthorized_dm_for_pairing(source). Checking it against main during the rebase turned up a second, larger problem in the same hunk: the diff also deleted _should_pass_unauthorized_dm_for_pairing and _source_from_reaction_for_auth from the adapter without putting either in the mixin, so they would have disappeared from the tree entirely rather than just changing behavior. Both stay on the adapter now, and _is_user_authorized_from_message is byte for byte main's current version, fallthrough included. That was my error and it should not have reached you.

The allowlist getters. Dropped from the mixin, as you suggested, with one adjustment. Rather than deleting them, I left all five on the adapter. TelegramConfigMentionMixin is not on main yet, so if this PR removed them from the adapter and the mixin dropped them too, they would be undefined until A5 lands. Leaving them in place means main keeps its current _scoped_gate_env versions, there is no duplicate for the MRO to arbitrate, and A5 can move them cleanly whenever it merges. Your reading of which copies were current was right: four of the five on main use _scoped_gate_env and the copies in my mixin were the older os.getenv ones, so merging as it stood would have reverted part of #72348.

The extraction is now four methods, not nine: _is_callback_user_authorized, _source_from_message_for_auth, _telegram_auth_env_configured and _is_user_authorized_from_message, each identical to main. I diffed every definition in main's adapter against the adapter plus mixin after the rebase and nothing is missing.

Two smaller things. The adapter's from gateway.authz_mixin import _coerce_allow_set had been replaced by the mixin import rather than joined to it, and _should_pass_unauthorized_dm_for_pairing still needs it, so that import is restored. _scoped_gate_env stays in the adapter and the mixin reaches it through a small delegating helper with the import deferred to call time, since copying an authorization gate reader is exactly the kind of duplicate that drifts.

I also changed the mixin's logger from a hard-coded "plugins.platforms.telegram.adapter" to f"{__package__}.adapter". That is outside what you raised, but @teknium1 flagged the identical hard-coded name on the Discord sibling (#75735) because the plugin manager loads directory plugins under hermes_plugins.<slug>, and it seemed worse to knowingly ship the same defect here. Happy to split it out if you would rather keep this PR to the auth change.

543 tests pass across the telegram gateway files plus test_discord_gate_isolation. Before the fix that same set was 529 passing and 14 failing, including the three pairing cases you named.

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

Labels

area/auth Authentication, OAuth, credential pools comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract the authorization cluster from TelegramAdapter (god-file decomposition)

5 participants