fix(discord): scope auto_thread/reactions/allow_mentions/reply_to_mode per profile - #100435
nftpoetrist wants to merge 1 commit into
Conversation
…e per profile _apply_yaml_config()'s YAML->env bridge for auto_thread, reactions, allow_mentions.*, and reply_to_mode wrote to process-global os.environ unconditionally (first-writer-wins, no _skip_env_bridge check), unlike the auth-gate keys (allow_from, allowed_roles, allow_all_users, free_response_channels, ignored_channels, allowed_channels, no_thread_channels) already fixed for this under issue NousResearch#72348. Under gateway.multiplex_profiles, a secondary profile's own config.yaml settings would leak into shared env for every other Discord adapter in the process to inherit — most notably allow_mentions.everyone: true, which would let every profile's bot ping @everyone/@here on its own server. reply_to_mode is the reverse-direction case: a secondary profile's own setting leaking into the (unscoped) default profile's read. The read sides (_reactions_enabled, the inline auto_thread check in _handle_message, _build_allowed_mentions) also read raw os.getenv/os.environ directly with no per-profile fallback at all. Fixes both directions, reusing established per-profile machinery instead of inventing a new mechanism: - _apply_yaml_config: gate the auto_thread/reactions/allow_mentions/ reply_to_mode env writes with the existing _skip_env_bridge check, and seed auto_thread/reactions/allow_mentions into PlatformConfig.extra (they weren't seeded at all before, so a scoped profile's own YAML value would otherwise have nowhere to go). - _reactions_enabled / new _auto_thread_enabled: use the existing self._gate_raw() per-profile accessor (the same one _get_allowed_channels etc. already use) instead of raw os.getenv, and add both env vars to _GATE_ENV_KEYS so they're captured in the connect()-time per-profile snapshot. - _build_allowed_mentions: accept an optional extra param (call site now passes self.config.extra), and use _scoped_gate_env (the same helper the gate accessors are built on) instead of raw os.getenv for each of the four DISCORD_ALLOW_MENTION_* flags, falling back to extra["allow_mentions"] when unset. Existing bare _build_allowed_mentions() calls (tests, and any future caller with no adapter instance) are unaffected — extra defaults to None, preserving the exact prior env-only behavior. Extends tests/plugins/platforms/test_discord_gate_isolation.py (the established test file for issue NousResearch#72348's bug class) with 16 new tests mirroring its existing conventions.
Another consistent application of the #72348 scope-isolation pattern to the Discord adapter: Points worth noting:
Verdict: LGTM |
… process env under multiplex Under gateway.multiplex_profiles every secondary profile's config loads inside _profile_runtime_scope, yet every apply_yaml_config_fn hook (feishu, matrix, whatsapp, slack, dingtalk, discord non-gate keys, telegram non-gate keys) and gateway/config_loader.py::bridge_core_env_settings still wrote os.environ there. First-writer-wins: the first secondary with a require_mention / allowlist / allow_bots / reactions block made that policy the DEFAULT profile's (live: TELEGRAM_REQUIRE_MENTION written from a secondary load), and secondaries read the default's env for the same keys. - gateway/platforms/_shared.py::yaml_env_setter: the one env-write shape for YAML->env bridges — env wins, skipped under an active secondary scope. - Every hook now seeds its values into the profile's PlatformConfig.extra and uses yaml_env_setter; bridge_core_env_settings seeds telegram/signal require_mention into extra and skips the env write under scope. - Readers that bypassed extra/scope now consult extra first (matrix flags + session_scope, slack reactions, telegram reactions/mention_patterns/_extra_bool, discord reactions/auto_thread/history_backfill/approval_mentions/allow_mentions, feishu allow_bots, dingtalk mention_patterns, signal require_mention). Salvages the shape of PR #100604 (whatsapp, earliest report #80099), #100435 (discord) and #100448 (telegram) by @nftpoetrist on top of current main. Fixes #80099 Co-authored-by: nftpoetrist <264138787+nftpoetrist@users.noreply.github.com>
|
Superseded by #108440 (on |
What & why
_apply_yaml_config()'s YAML→env bridge forauto_thread,reactions,allow_mentions.*, andreply_to_modewrites to process-globalos.environunconditionally (first-writer-wins, no_skip_env_bridgecheck) — unlike the auth-gate keys (allow_from,allowed_roles,allow_all_users,free_response_channels,ignored_channels,allowed_channels,no_thread_channels) already fixed for this exact bug class under issue #72348.Under
gateway.multiplex_profiles, a secondary profile's ownconfig.yamlsettings leak into shared env for every other Discord adapter in the process to inherit. Worst case:allow_mentions.everyone: trueon one profile silently lets every profile's bot ping@everyone/@hereon its own server — Discord bots default to parsing those pings, and this adapter's own docstring explains the safe-default exists specifically to stop LLM output/echoed content from doing that.reply_to_modeis the reverse-direction case: a secondary profile's own setting leaks into the (unscoped) default profile's read.The read sides (
_reactions_enabled, the inlineauto_threadcheck in_handle_message,_build_allowed_mentions) also read rawos.getenv/os.environdirectly with no per-profile fallback at all.Fix
Reuses the established per-profile machinery from issue #72348 instead of inventing a new mechanism:
_apply_yaml_config: gates theauto_thread/reactions/allow_mentions/reply_to_modeenv writes with the existing_skip_env_bridgecheck (_profile_scoped_config_load()), and seedsauto_thread/reactions/allow_mentionsintoPlatformConfig.extra(they weren't seeded at all before, so a scoped profile's own YAML value would otherwise have nowhere to go)._reactions_enabled/ new_auto_thread_enabled: use the existingself._gate_raw()per-profile accessor (the same one_get_allowed_channelsetc. already use) instead of rawos.getenv, and add both env vars to_GATE_ENV_KEYSso they're captured in theconnect()-time per-profile snapshot._build_allowed_mentions: accepts an optionalextraparam (the call site now passesself.config.extra), and uses_scoped_gate_env(the same helper the gate accessors are built on) instead of rawos.getenvfor each of the fourDISCORD_ALLOW_MENTION_*flags, falling back toextra["allow_mentions"]when unset. Existing bare_build_allowed_mentions()calls (tests, and any future caller with no adapter instance) are unaffected —extradefaults toNone, preserving the exact prior env-only behavior.Tests
Extended
tests/plugins/platforms/test_discord_gate_isolation.py(the established test file for issue #72348's bug class) with 16 new tests mirroring its existing conventions (two-adapter isolation, snapshot-vs-extra precedence, env-does-not-leak-into-snapshotted-adapter, and_apply_yaml_configscoped-vs-single-profile write behavior).tests/plugins/platforms/test_discord_gate_isolation.py— 37 passedtests/gateway/test_discord_*.py,tests/tools/test_discord_*.py,tests/hermes_cli/test_discord_*.py,tests/plugins/test_discord_*.py(full Discord suite) — 346 passed, 3 pre-existing failures intest_discord_send.pyconfirmed independent of this change (pass 11/11 when run standalone, both with and without this fix — a test-order-pollution artifact of running the whole suite together, not a regression)uv pip install "discord.py[voice]==2.7.1"per the standing optional-dependency gotcha, to get real dynamic verification instead ofimportorskipskips on theAllowedMentions-dependent testsCompetitor check
Searched
DISCORD_AUTO_THREAD,DISCORD_REACTIONS,DISCORD_ALLOW_MENTION,DISCORD_REPLY_TO_MODE, "discord multiplex allow_mentions" (all PR/issue states) — no PR targets this scoping bug. One open PR (#68473) touches the same_handle_messageregion (adds alogger.debugcall in the neighboringrequire_mentiondrop path) — different lines, different concern, no overlap.