fix(mattermost): harden delivery hygiene
PROBLEM: Mattermost threads can become invalid or enormous, exposing two failure modes: internal scratch/reasoning/commentary displays could leak into persistent Mattermost threads via global display toggles, while rejected threaded user-visible replies could disappear unless every failed send fell back flat. A broad flat fallback would pollute channels with tool/status/progress noise. SOLUTION: Require explicit Mattermost platform opt-in for scratch displays, keep using the existing notify=True metadata marker for user-visible final text/media/file replies, and allow the Mattermost plugin adapter to flat-fallback only notify-worthy sends whose threaded POST failure looks like a broken root/thread. Keep tool/status/progress and other non-notify sends thread-strict. Add regression tests for display opt-in, notify-only broken-thread fallback, generic API failure suppression, and stream notify metadata. Verification: tests/gateway/test_mattermost.py tests/gateway/test_stream_consumer.py tests/gateway/test_stream_consumer_thread_routing.py tests/gateway/test_stream_consumer_fresh_final.py tests/gateway/test_stream_consumer_draft.py; tests/gateway/test_session_api.py tests/gateway/test_status_command.py tests/gateway/test_resume_command.py tests/hermes_cli/test_commands.py; py_compile touched gateway files; git diff --check. Session: Mattermost thread 6qg8e9dd1pd9pkhi74xyaa1mry, 2026-06-01.
This commit is contained in:
Wolfram Ravenwolf
Teknium
parent
925b0d1ab5
commit
16fc717091
7 changed files with 329 additions and 53 deletions
|
|
@ -6,7 +6,10 @@ import pytest
|
|||
from unittest.mock import MagicMock, patch, AsyncMock
|
||||
|
||||
from gateway.config import Platform, PlatformConfig
|
||||
from gateway.run import _resolve_progress_thread_id
|
||||
from gateway.run import (
|
||||
_resolve_gateway_display_bool,
|
||||
_resolve_progress_thread_id,
|
||||
)
|
||||
|
||||
|
||||
class TestMattermostProgressThreadRouting:
|
||||
|
|
@ -32,6 +35,97 @@ class TestMattermostProgressThreadRouting:
|
|||
) is None
|
||||
|
||||
|
||||
class TestMattermostDisplayHygiene:
|
||||
def test_mattermost_requires_platform_opt_in_for_interim_assistant_messages(self):
|
||||
"""Global interim commentary must not make Mattermost leak scratch notes."""
|
||||
user_config = {"display": {"interim_assistant_messages": True}}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"mattermost",
|
||||
"interim_assistant_messages",
|
||||
default=True,
|
||||
platform=Platform.MATTERMOST,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is False
|
||||
|
||||
def test_mattermost_platform_opt_in_can_enable_interim_assistant_messages(self):
|
||||
"""Mattermost can still opt into commentary explicitly per platform."""
|
||||
user_config = {
|
||||
"display": {
|
||||
"interim_assistant_messages": False,
|
||||
"platforms": {
|
||||
"mattermost": {"interim_assistant_messages": True},
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"mattermost",
|
||||
"interim_assistant_messages",
|
||||
default=True,
|
||||
platform=Platform.MATTERMOST,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is True
|
||||
|
||||
def test_mattermost_requires_platform_opt_in_for_thinking_progress(self):
|
||||
"""Global thinking_progress must not surface internal analysis in Mattermost."""
|
||||
user_config = {"display": {"thinking_progress": True}}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"mattermost",
|
||||
"thinking_progress",
|
||||
default=False,
|
||||
platform=Platform.MATTERMOST,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is False
|
||||
|
||||
def test_mattermost_requires_platform_opt_in_for_show_reasoning(self):
|
||||
"""Global show_reasoning must not prepend scratch reasoning in Mattermost."""
|
||||
user_config = {"display": {"show_reasoning": True}}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"mattermost",
|
||||
"show_reasoning",
|
||||
default=False,
|
||||
platform=Platform.MATTERMOST,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is False
|
||||
|
||||
def test_mattermost_platform_opt_in_can_enable_show_reasoning(self):
|
||||
user_config = {
|
||||
"display": {
|
||||
"show_reasoning": False,
|
||||
"platforms": {"mattermost": {"show_reasoning": True}},
|
||||
}
|
||||
}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"mattermost",
|
||||
"show_reasoning",
|
||||
default=False,
|
||||
platform=Platform.MATTERMOST,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is True
|
||||
|
||||
def test_global_thinking_progress_still_applies_to_other_platforms(self):
|
||||
"""The Mattermost guard must not silently neuter Telegram/other chats."""
|
||||
user_config = {"display": {"thinking_progress": True}}
|
||||
|
||||
assert _resolve_gateway_display_bool(
|
||||
user_config,
|
||||
"telegram",
|
||||
"thinking_progress",
|
||||
default=False,
|
||||
platform=Platform.TELEGRAM,
|
||||
require_platform_override_for={Platform.MATTERMOST},
|
||||
) is True
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Platform & Config
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -347,6 +441,24 @@ class TestMattermostSend:
|
|||
payload = self.adapter._api_post.call_args_list[0][0][1]
|
||||
assert payload["root_id"] == "root_post"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_progress_send_with_invalid_thread_root_never_falls_back_flat(self):
|
||||
"""Tool/status/progress bubbles must stay quiet when the thread is broken."""
|
||||
self.adapter._reply_mode = "thread"
|
||||
self.adapter._api_get = AsyncMock(return_value={"id": "bad_root", "root_id": ""})
|
||||
self.adapter._api_post = AsyncMock(return_value={})
|
||||
|
||||
result = await self.adapter.send(
|
||||
"channel_1",
|
||||
"⚙️ terminal...",
|
||||
metadata={"thread_id": "bad_root"},
|
||||
)
|
||||
|
||||
assert result.success is False
|
||||
assert self.adapter._api_post.call_count == 1
|
||||
payload = self.adapter._api_post.call_args_list[0][0][1]
|
||||
assert payload["root_id"] == "bad_root"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_api_failure(self):
|
||||
"""When API returns error, send should return failure."""
|
||||
|
|
|
|||
|
|
@ -106,6 +106,42 @@ class TestInitialReplyToId:
|
|||
assert call_kwargs["metadata"] == {**metadata, "expect_edits": True}
|
||||
assert metadata == {"thread_id": "omt_topic789"}
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_final_first_send_marks_metadata_notify_true(self):
|
||||
"""Final streaming sends should use the existing notify=True marker."""
|
||||
adapter = _make_adapter()
|
||||
consumer = GatewayStreamConsumer(
|
||||
adapter,
|
||||
"chat_123",
|
||||
metadata={"thread_id": "root_post_123"},
|
||||
initial_reply_to_id="reply_post_456",
|
||||
)
|
||||
|
||||
await consumer._send_or_edit("Final answer", finalize=True)
|
||||
|
||||
call_kwargs = adapter.send.call_args[1]
|
||||
metadata = call_kwargs["metadata"]
|
||||
assert metadata["thread_id"] == "root_post_123"
|
||||
assert metadata["notify"] is True
|
||||
assert "delivery_kind" not in metadata
|
||||
assert "allow_flat_fallback" not in metadata
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_nonfinal_first_send_does_not_mark_notify(self):
|
||||
"""Preview/interim streaming sends must not be notify-worthy."""
|
||||
adapter = _make_adapter()
|
||||
consumer = GatewayStreamConsumer(
|
||||
adapter,
|
||||
"chat_123",
|
||||
metadata={"thread_id": "root_post_123"},
|
||||
initial_reply_to_id="reply_post_456",
|
||||
)
|
||||
|
||||
await consumer._send_or_edit("Preview", finalize=False)
|
||||
|
||||
metadata = adapter.send.call_args[1]["metadata"]
|
||||
assert metadata == {"thread_id": "root_post_123", "expect_edits": True}
|
||||
|
||||
|
||||
class TestOverflowFirstMessage:
|
||||
"""Verify thread routing is preserved when the first message overflows."""
|
||||
|
|
|
|||
|
|
@ -134,7 +134,7 @@ async def test_stream_consumer_fallback_sends_tail_after_partial_overflow():
|
|||
|
||||
adapter.send.assert_awaited_once()
|
||||
assert adapter.send.await_args.kwargs["content"] == "world"
|
||||
assert adapter.send.await_args.kwargs["metadata"] == {"thread_id": "77"}
|
||||
assert adapter.send.await_args.kwargs["metadata"] == {"thread_id": "77", "notify": True}
|
||||
adapter.delete_message.assert_not_awaited()
|
||||
assert consumer.final_response_sent is True
|
||||
assert consumer.final_content_delivered is True
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ async def test_base_adapter_routes_telegram_flac_media_tag_to_document_sender(tm
|
|||
adapter.send_document.assert_awaited_once_with(
|
||||
chat_id="chat-1",
|
||||
file_path=str(media_file),
|
||||
metadata=None,
|
||||
metadata={"notify": True},
|
||||
)
|
||||
adapter.send_voice.assert_not_awaited()
|
||||
|
||||
|
|
@ -95,7 +95,7 @@ async def test_base_adapter_routes_non_voice_telegram_ogg_media_tag_to_document_
|
|||
adapter.send_document.assert_awaited_once_with(
|
||||
chat_id="chat-1",
|
||||
file_path=str(media_file),
|
||||
metadata=None,
|
||||
metadata={"notify": True},
|
||||
)
|
||||
adapter.send_voice.assert_not_awaited()
|
||||
|
||||
|
|
@ -116,7 +116,7 @@ async def test_base_adapter_routes_voice_tagged_telegram_ogg_media_tag_to_voice_
|
|||
adapter.send_voice.assert_awaited_once_with(
|
||||
chat_id="chat-1",
|
||||
audio_path=str(media_file),
|
||||
metadata=None,
|
||||
metadata={"notify": True},
|
||||
)
|
||||
adapter.send_document.assert_not_awaited()
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue