fix(mattermost): keep progress notices in threads - #45861
Conversation
50cd084 to
53371a4
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the Mattermost routing work. The thread-routing portion is now already covered on current main by 5a0e0d35b (gateway/run.py:488-496, plugins/platforms/mattermost/adapter.py:163-176, and tests/gateway/test_mattermost.py:16-29).
Problems
- The PR also adds an interactive command-approval callback system beginning at
plugins/platforms/mattermost/adapter.py:399in the PR diff. That is a separate security-boundary feature from the stated progress-threading fix and is not described in the PR summary.
Suggested changes
- Any salvage should omit the already-landed routing portion.
- Please handle the approval-action work as a focused change with explicit deployment/configuration documentation and dedicated callback-security review.
This is an automated hermes-sweeper review.
| @@ -300,6 +399,176 @@ async def send( | |||
|
|
|||
There was a problem hiding this comment.
This starts a separate interactive approval callback feature, while the PR summary describes only progress-thread routing. Since approval callbacks cross a security boundary, please split this into a focused change with explicit deployment/configuration documentation and dedicated review; the routing portion is already implemented on current main.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the Mattermost top-level-channel progress/status threading gap: #36916 (closed) and #45861 (open, target). Both diffs implement a near-identical fix — anchoring progress/status/heartbeat sends to the triggering post's thread and teaching MattermostAdapter.send() to honor metadata['thread_id'] — and both were reviewed as already implemented on current main via commit 5a0e0d3. #45861 additionally bundles an ~460-line interactive command-approval-button subsystem (HMAC-signed callback contexts, a new aiohttp callback server, allowlist auth) that is unrelated to and undisclosed in its stated progress-threading fix.
Related pull requests
- #36916 [closed]
duplicate— (+153/-12) — closed as implemented_on_main: adds _progress_thread_id_for_source() and a metadata['thread_id'] fallback in MattermostAdapter.send() so top-level channel progress/status bubbles anchor to the triggering post; teknium1 [contributor:7327 commits] confirmed this is superseded by 5a0e0d3 already on main (gateway/run.py:17470-17482, adapter.py:362-367). Still relevant as the reference diff documenting the shape of the landed fix. - #45861
related— (+635/-25) — keep_open review (salvageability=medium) from teknium1 [contributor:7327 commits]: the routing portion (_progress_thread_target_for_source, adapter metadata['thread_id'] fallback) duplicates the same already-landed main fix (5a0e0d3). The PR also introduces an undisclosed Mattermost interactive-approval-button subsystem (signed callback contexts, new HTTP action server, allowlist auth) that the reviewer flags as a separate security-boundary feature needing its own focused PR and dedicated callback-security review before merge.
Duplicates
#36916 and the routing-fix portion of #45861 are essentially the same change (top-level Mattermost progress/status thread anchoring plus the metadata['thread_id'] adapter fallback), and both duplicate logic already shipped on main in 5a0e0d3.
Suggested consolidation
In line with teknium1's keep_open review on #45861: do not merge #45861 as-is — its routing fix is redundant with main's 5a0e0d3, and the bundled approval-action feature (adapter.py:399+) must be split into its own PR with explicit deployment documentation and dedicated security review before consideration. #36916 stays closed and can be discarded beyond its reference value. Recommend keeping #45861 open pending the author dropping the redundant routing hunks and splitting out the approval-button work into a separately reviewed PR.
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
subgraph Dup36916 ["PRs duplicating each other"]
P36916["PR #36916 (closed)"]
P45861["PR #45861 (open)"]
end
class P36916 closed
class P45861 open
class P45861 target
click P36916 "https://github.com/NousResearch/hermes-agent/pull/36916"
click P45861 "https://github.com/NousResearch/hermes-agent/pull/45861"
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 or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 48 kB of PR diffs, 5 kB of issue/PR text, 2 kB of discussion (2 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
Fixes Mattermost gateway progress/status/warning messages escaping as flat channel posts when the user starts from a top-level channel message.
source.thread_idis empty.reply_to, soMATTERMOST_REPLY_MODE=threadcan turn it into a Mattermostroot_id.MattermostAdapter.send()now honors genericmetadata["thread_id"]as a fallback thread anchor for synthetic sends when explicitreply_tois absent.Related PRs
Related to #36916 and #41640. Those PRs cover overlapping Mattermost thread-routing issues with broader side-channel/delivery hygiene. This PR is a smaller current-main focused fix for the specific Slack-vs-Mattermost regression where top-level Mattermost channel prompts caused warning/progress/status bubbles to land in the parent channel instead of the triggering post's thread.
Test plan
uv run --with pytest --with pytest-asyncio --with aiohttp pytest tests/gateway/test_mattermost.py tests/gateway/test_mattermost_thread_routing.py tests/gateway/test_goal_status_notice.py -quv run --with pytest --with pytest-asyncio --with aiohttp python -m py_compile gateway/run.py plugins/platforms/mattermost/adapter.py tests/gateway/test_mattermost.py tests/gateway/test_mattermost_thread_routing.pygit diff --checkNotes
reply_tostill takes precedence over metadata, and Mattermostroot_idis still gated behindreply_mode == "thread".