feat(mattermost): thread context seeding, allowlist-filtered - #43805
feat(mattermost): thread context seeding, allowlist-filtered#43805crisap94 wants to merge 2 commits into
Conversation
After the bot is @mentioned in a thread, subsequent messages in that thread auto-trigger it without a new @mention (Slack parity). Opt out with MATTERMOST_STRICT_MENTION=true. - _mentioned_threads tracks engaged threads; _has_active_session_for_thread passes the adapter's real chat_type so the session key matches what handle_message persists. - Includes the root-post session-continuity fix (same as NousResearch#37144 by @brendanstennett): a Mattermost root post has an empty root_id, so in thread mode thread_id is seeded from the post's own id to keep the root and its replies on one session. Auto-response depends on this, so it is included here with credit; happy to drop it if NousResearch#37144 lands first. - Auto-response does not bypass authz: every message is still authorized by author downstream. Adds TestMattermostThreadSessionContinuity + TestMattermostInThreadAutoResponse and documents MATTERMOST_STRICT_MENTION. Related: NousResearch#37144 (session fix, credited)
When @mentioned inside an existing thread for the first time, seed the prior
thread history as context (GET /posts/{root}/thread) via MessageEvent.channel_context,
so the agent sees the whole conversation, not just the triggering message.
Security: the authz layer only checks the *triggering* message's author;
injected channel_context bypasses it. So seeded history is filtered to authors
in MATTERMOST_ALLOWED_USERS (or everyone when *_ALLOW_ALL_USERS is set) to avoid
leaking a non-allowlisted user's messages into the model. MATTERMOST_THREAD_CONTEXT
controls the policy: allowlisted (default), off, or all (full thread without
widening who may invoke the bot). Fails open on fetch error.
Stacked on the in-thread auto-response change (uses _has_active_session_for_thread
as the first-turn guard).
Adds TestMattermostThreadContext and documents MATTERMOST_THREAD_CONTEXT.
Supersedes NousResearch#38362, NousResearch#38152 (thread context without the allowlist filter).
c603a2d to
e8b02c9
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real Mattermost context gap: current main creates the event without thread history at plugins/platforms/mattermost/adapter.py:949-958, while gateway/run.py:10394-10395 already consumes channel_context.
Problems
plugins/platforms/mattermost/adapter.py:723-746reimplements only part of authorization. It misses the global allowlist and pairing-store grants used bygateway/authz_mixin.py:454-465. Current main’s adapter callback (gateway/platforms/base.py:2838-2869,gateway/run.py:8785-8815) exists specifically to reuse that full chain for fetched context.- The cache at proposed
adapter.py:795-797is keyed only by thread root although the rendered result excludescurrent_post_idat lines 811-813. A second user’s first turn within the TTL can therefore receive stale context that omits the first user’s now-prior post. The added tests (tests/gateway/test_mattermost.py:946-1010) do not cover that sequence. - The two new controls are documented but absent from the Mattermost setup manifest (
plugins/platforms/mattermost/plugin.yaml:15-49).
Suggested changes
- Use
_is_sender_authorized(...)and test global/pairing authorization. - Make cache results trigger-specific or regenerate them from cached raw posts; test two first-session users.
- Register both controls in the Mattermost plugin manifest.
Automated hermes-sweeper review.
| return True | ||
| allowed = { | ||
| u.strip() | ||
| for u in os.getenv("MATTERMOST_ALLOWED_USERS", "").split(",") |
There was a problem hiding this comment.
This duplicates only a subset of gateway authorization: it misses GATEWAY_ALLOWED_USERS and pairing-store grants. Current main provides _is_sender_authorized(author, chat_type=chat_type, chat_id=channel_id) specifically so fetched context uses the complete GatewayRunner._is_user_authorized chain; please use that callback instead of re-parsing env vars.
| leaving the agent with just the triggering message. | ||
| """ | ||
| now = time.monotonic() | ||
| cached = self._thread_context_cache.get(thread_root_id) |
There was a problem hiding this comment.
The cached string depends on current_post_id because the loop excludes that post, but this key is only thread_root_id. A later first turn by another user during the TTL receives the earlier rendered string and loses the first trigger as prior context. Cache raw posts or make this result trigger-specific, with a two-user regression test.
| with patch.dict(os.environ, {"MATTERMOST_THREAD_CONTEXT": "bogus"}): | ||
| assert self.adapter._thread_context_mode() == "allowlisted" | ||
|
|
||
| @pytest.mark.asyncio |
There was a problem hiding this comment.
Please add regression coverage for two authorized users entering the same thread within the cache TTL: the second user’s seeded context must include the first user’s earlier triggering post.
What does this PR do?
Seeds prior thread history as context when the bot is
@mentionedinside an existing thread for the first time, so the agent understands the conversation it was pulled into — not just the single message it was tagged in. History is injected viaMessageEvent.channel_context(consumed ingateway/run.py).Security: the allowlist must apply to seeded context
The user allowlist is enforced in
gateway/authz_mixin.py::_is_user_authorized, per message, by the triggering author — downstream of the adapter. Injectedchannel_contextrides inside the authorized user's message and is not re-checked by authz. So seeding the whole thread verbatim would feed a non-allowlisted user's messages into the model (an unauthorized-content / prompt-injection vector).This PR filters seeded history to authors in
MATTERMOST_ALLOWED_USERS(or everyone when*_ALLOW_ALL_USERSis set), and addsMATTERMOST_THREAD_CONTEXTto control the policy explicitly:allowlisted(default)offallThis filtering is the piece the existing thread-context PRs (#38362, #38152) do not have.
Type of Change
Changes Made
All in
plugins/platforms/mattermost/adapter.py(the thread-context commit):_fetch_thread_context()— fetchesGET /posts/{root}/thread, fails open via the existing_api_get, TTL-cached, excludes the trigger/bot/system posts, filters to allowlisted authors, injects viachannel_context._thread_context_author_allowed()— mirrors the authz allowlist; honorsMATTERMOST_THREAD_CONTEXT=all._thread_context_mode()—off/allowlisted/all._has_active_session_for_thread(from feat(mattermost): in-thread auto-response (Slack parity) #43804) so it runs once per thread.TestMattermostThreadContext(filter, allow-all, knob off/all, fail-open, DM).MATTERMOST_THREAD_CONTEXT+ the allowlist-context warning.How to Test
MATTERMOST_ALLOWED_USERS=<you>.@mentionthe bot in a thread with prior messages → reply reflects them.MATTERMOST_THREAD_CONTEXT=off→ no seeding;=all→ full thread seeded without opening invocation.pytest tests/gateway/test_mattermost.py -q→ 63 passed.Checklist
feat(mattermost): …)pytest tests/gateway/test_mattermost.py -qpasses (63)cli-config.yaml.example— N/A (env/config.extra-driven)