fix(mattermost): scope url/reply_mode/require_mention/free_response_channels/allowed_channels to the active profile under multiplexing - #100647
Closed
nftpoetrist wants to merge 1 commit into
Conversation
…hannels/allowed_channels to the active profile under multiplexing MattermostAdapter.__init__, validate_mattermost_config, _standalone_send, and _handle_ws_event's mention-gating block all read MATTERMOST_URL/ MATTERMOST_REPLY_MODE/MATTERMOST_REQUIRE_MENTION/ MATTERMOST_FREE_RESPONSE_CHANNELS/MATTERMOST_ALLOWED_CHANNELS via raw os.getenv -- only MATTERMOST_TOKEN was already scoped via _get_scoped_secret. _apply_yaml_config additionally wrote MATTERMOST_REQUIRE_MENTION/MATTERMOST_FREE_RESPONSE_CHANNELS/ MATTERMOST_ALLOWED_CHANNELS into the process-global os.environ unconditionally (guarded only by `not os.getenv(...)`, first-writer-wins), the same apply_yaml_config_fn bug class already fixed for the Discord/Telegram/WhatsApp/DingTalk adapters in this series. Under gateway.multiplex_profiles, os.environ holds the DEFAULT profile's env-bridge output. A secondary profile with its own (or no) Mattermost config could silently connect to the default profile's server, thread its replies per the default profile's reply_mode, or -- since _handle_ws_event's mention-gating block runs on every LIVE inbound message, not just at construction -- have its require_mention/ free_response_channels/allowed_channels decisions driven by the default profile's settings for the adapter's entire runtime lifetime. Fix, mirroring the WhatsApp/DingTalk apply_yaml_config_fn pattern: - Add _profile_scoped_config_load() (same helper as DingTalk). - Rewrite _apply_yaml_config to skip the env-bridge write under a multiplexed secondary profile's scope, and instead return the YAML values as a dict merged into this profile's own PlatformConfig.extra. - Make require_mention/free_response_channels read extra first (matching the existing allowed_channels precedent), falling back to _get_scoped_secret() instead of raw os.getenv when extra is absent -- fixing a residual gap the DingTalk fix (NousResearch#100615, this series' item 6) left in its own analogous extra-first-with-raw-fallback read sites (_dingtalk_require_mention et al. still fall back to bare os.getenv). - Switch __init__'s url/reply_mode, validate_mattermost_config's url, and _standalone_send's url to _get_scoped_secret(). - Leave check_mattermost_requirements() (no longer reads any MATTERMOST_* var on current main -- just an aiohttp-importability probe) and _is_connected() (already scope-aware via hermes_cli.gateway.get_env_value, which itself routes through agent.secret_scope.get_secret) untouched. Adds a new TestMultiplexProfileScope class to tests/gateway/test_mattermost.py (7 tests) mirroring the fixture/assertion style established in tests/gateway/test_line_plugin.py's TestMultiplexProfileScope, plus two tests exercising _apply_yaml_config's new seeded-dict return directly. Mutation-verified: stashed the production fix and confirmed 5 of 7 new tests fail against pre-fix code (the other 2 are non-differentiating regression guards -- extra-wins-over-env and unscoped-default-profile- precedence -- which correctly pass either way). Restored the fix; all 30 tests in the file, the plugin-setup test, and the full 75-test tests/gateway/test_adapter_startup_secret_scope.py suite pass.
Contributor
Mattermost multiplex-scope fix: URL/reply_mode/require_mention/free_response_channels/allowed_channels now read via scope, and
|
Collaborator
|
Thanks @nftpoetrist — Merged via #101252 (2e25b47) on current main. Your commits from this PR were cherry-picked onto the salvage branch with your git authorship preserved, so the credit is yours in Closing this PR since the work is now on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MattermostAdapter.__init__,validate_mattermost_config,_standalone_send, and_handle_ws_event's mention-gating block all readMATTERMOST_URL/MATTERMOST_REPLY_MODE/MATTERMOST_REQUIRE_MENTION/MATTERMOST_FREE_RESPONSE_CHANNELS/MATTERMOST_ALLOWED_CHANNELSvia rawos.getenv()— onlyMATTERMOST_TOKENwas already scoped via_get_scoped_secret()._apply_yaml_configadditionally wroteMATTERMOST_REQUIRE_MENTION/MATTERMOST_FREE_RESPONSE_CHANNELS/MATTERMOST_ALLOWED_CHANNELSinto the process-globalos.environunconditionally (guarded only bynot os.getenv(...), first-writer-wins) — the sameapply_yaml_config_fnbug class already fixed for the Discord/Telegram/WhatsApp/DingTalk adapters in this series.gateway.multiplex_profiles,os.environholds the DEFAULT profile's env-bridge output. A secondary profile with its own (or no) Mattermost config could silently connect to the default profile's server, thread its replies per the default profile'sreply_mode, or — since_handle_ws_event's mention-gating block runs on every live inbound message, not just at construction — have itsrequire_mention/free_response_channels/allowed_channelsdecisions driven by the default profile's settings for the adapter's entire runtime lifetime.Fix
Mirrors the WhatsApp/DingTalk
apply_yaml_config_fnpattern:_profile_scoped_config_load()(same helper as DingTalk)._apply_yaml_configto skip the env-bridge write under a multiplexed secondary profile's scope, and instead return the YAML values as a dict merged into this profile's ownPlatformConfig.extra.require_mention/free_response_channelsreadextrafirst (matching the existingallowed_channelsprecedent), falling back to_get_scoped_secret()instead of rawos.getenvwhenextrais absent — fixing a residual gap the DingTalk fix (fix(dingtalk): scope require_mention/allowlists per profile #100615, item 6 of this series) left in its own analogous extra-first-with-raw-fallback read sites (_dingtalk_require_mentionet al. still fall back to bareos.getenvon an extra miss — flagging this as a follow-up worth a look, not fixed here since it's a different file/PR).__init__'surl/reply_mode,validate_mattermost_config'surl, and_standalone_send'surlto_get_scoped_secret().check_mattermost_requirements()(no longer reads anyMATTERMOST_*var on currentmain— just an aiohttp-importability probe) and_is_connected()(already scope-aware viahermes_cli.gateway.get_env_value, which itself routes throughagent.secret_scope.get_secret) untouched.Tests
Added
TestMultiplexProfileScope(7 tests) totests/gateway/test_mattermost.py, mirroring the fixture/assertion style established intests/gateway/test_line_plugin.py's class of the same name, plus 2 tests exercising_apply_yaml_config's new seeded-dict return directly.Mutation-verified: stashed the production fix and confirmed 5 of 7 new tests fail against pre-fix code (the other 2 are non-differentiating regression guards — extra-wins-over-env and unscoped-default-profile-precedence — which correctly pass either way). Restored the fix; all 30 tests in the file, the plugin-setup test, and the full 75-test
tests/gateway/test_adapter_startup_secret_scope.pysuite pass.Competitor check
This file is a hotspot with several open PRs; searched
MATTERMOST_URL,mattermost multiplex,mattermost scope,mattermost require_mention. None address the multiplex-scoping concern fixed here — all are different concerns with two real (but reconcilable) line-level overlaps and two adjacency-only overlaps:reply_modebridge into_apply_yaml_config— the same function this PR restructures. A rebase needs to port their newreply_modebranch into the_skip_env_bridge/seeded-dict shape this PR introduces (straightforward: same pattern as the other three fields).self._reply_mode = (...)expression in__init__, wrapping it in a validation helper and changing the default constant — different concern (validation) from this PR'sos.getenv→_get_scoped_secretswap. Trivially combinable on rebase.require_mention/free_response_channels/allowed_channelsblock and moves an unrelated code block — textual adjacency, no line-level overlap.has_mentioncheck further down in the same function, using the old single-linefree_channels = ...form as diff context (this PR restructures that line) — needs a rebase to reconcile the anchor, but no semantic clash.check_mattermost_requirements()body) — no overlap.No open or merged PR touches the actual scope-leak fixed here.
Checklist
apply_yaml_config_fnpattern)