mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-24 16:54:43 +00:00
fix(slack): honor ignored channels before gateway dispatch
This commit is contained in:
parent
543941f709
commit
f8f5ce7da5
4 changed files with 338 additions and 5 deletions
|
|
@ -941,6 +941,48 @@ def _uses_telegram_observed_group_context(channel_prompt: Optional[str]) -> bool
|
|||
return bool(channel_prompt and _TELEGRAM_OBSERVED_CONTEXT_PROMPT_MARKER in channel_prompt)
|
||||
|
||||
|
||||
def _csv_or_list_to_set(raw: Any) -> set[str]:
|
||||
"""Normalize a config list or comma-separated scalar into a string set."""
|
||||
if raw is None:
|
||||
return set()
|
||||
if isinstance(raw, list):
|
||||
return {str(part).strip() for part in raw if str(part).strip()}
|
||||
s = str(raw).strip()
|
||||
if not s:
|
||||
return set()
|
||||
return {part.strip() for part in s.split(",") if part.strip()}
|
||||
|
||||
|
||||
def _slack_ignored_channels_from_gateway_config(config: Any) -> set[str]:
|
||||
"""Return Slack channels that the generic gateway must never dispatch.
|
||||
|
||||
The Slack adapter has the first-line drop, but this runner-level guard is
|
||||
intentionally duplicated as a fail-safe. If a future Slack code path, test
|
||||
hook, malformed event, or stale adapter instance bypasses the Slack plugin
|
||||
adapter, ignored channels still cannot reach auth, pairing, sessions, or
|
||||
the agent/home-channel prompt pipeline.
|
||||
"""
|
||||
platform_cfg = getattr(config, "platforms", {}).get(Platform.SLACK)
|
||||
raw = None
|
||||
if platform_cfg is not None:
|
||||
raw = getattr(platform_cfg, "extra", {}).get("ignored_channels")
|
||||
return _csv_or_list_to_set(raw)
|
||||
|
||||
|
||||
def _slack_parent_channel_id(chat_id: Any) -> str:
|
||||
"""Return the parent Slack channel from a possibly thread-scoped chat ID."""
|
||||
if not chat_id:
|
||||
return ""
|
||||
return str(chat_id).split(":", 1)[0]
|
||||
|
||||
|
||||
def _is_slack_ignored_channel(config: Any, chat_id: Any) -> bool:
|
||||
"""Check the generic Slack gateway blacklist for channel or thread IDs."""
|
||||
channel_id = _slack_parent_channel_id(chat_id)
|
||||
ignored = _slack_ignored_channels_from_gateway_config(config)
|
||||
return bool(channel_id and ("*" in ignored or channel_id in ignored))
|
||||
|
||||
|
||||
def _message_timestamps_enabled(user_config: Optional[dict]) -> bool:
|
||||
"""True when gateway.message_timestamps.enabled is opted in.
|
||||
|
||||
|
|
@ -10191,6 +10233,17 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
return
|
||||
|
||||
config = getattr(self, "config", None)
|
||||
if (
|
||||
config
|
||||
and getattr(source, "platform", None) == Platform.SLACK
|
||||
and _is_slack_ignored_channel(config, getattr(source, "chat_id", None))
|
||||
):
|
||||
logger.info(
|
||||
"Skipping Slack platform notice for configured ignored channel %s",
|
||||
getattr(source, "chat_id", None),
|
||||
)
|
||||
return
|
||||
|
||||
notice_delivery = "public"
|
||||
if config and hasattr(config, "get_notice_delivery"):
|
||||
notice_delivery = config.get_notice_delivery(source.platform)
|
||||
|
|
@ -10397,18 +10450,32 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
except Exception:
|
||||
logger.debug("reset_session_vars failed at handler entry", exc_info=True)
|
||||
|
||||
# Internal events (e.g. background-process completion notifications)
|
||||
# are system-generated and must skip user authorization.
|
||||
is_internal = bool(getattr(event, "internal", False))
|
||||
|
||||
# Ignored-channel guard runs FIRST — before startup-restore queueing,
|
||||
# plugin hooks, auth, and session setup — so a configured ignored
|
||||
# channel can never reach pairing/auth/session state (#51899).
|
||||
if (
|
||||
not is_internal
|
||||
and getattr(source, "platform", None) == Platform.SLACK
|
||||
and _is_slack_ignored_channel(self.config, getattr(source, "chat_id", None))
|
||||
):
|
||||
logger.info(
|
||||
"Dropping Slack message from configured ignored channel %s",
|
||||
getattr(source, "chat_id", None),
|
||||
)
|
||||
return None
|
||||
|
||||
if (
|
||||
getattr(self, "_startup_restore_in_progress", False)
|
||||
and not getattr(event, "internal", False)
|
||||
and not is_internal
|
||||
and not getattr(event, "_hermes_startup_restore_replay", False)
|
||||
):
|
||||
self._queue_startup_restore_event(event)
|
||||
return None
|
||||
|
||||
# Internal events (e.g. background-process completion notifications)
|
||||
# are system-generated and must skip user authorization.
|
||||
is_internal = bool(getattr(event, "internal", False))
|
||||
|
||||
# scale-to-zero (Phase 0, 0.B/F13): stamp the gateway-scoped last-inbound
|
||||
# clock for real (user-originated) inbound only. Internal/system events
|
||||
# (background-process completions, startup-restore replays) are NOT
|
||||
|
|
|
|||
|
|
@ -2389,6 +2389,31 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
except Exception as e: # pragma: no cover - defensive cleanup
|
||||
logger.debug("[Slack] status cleanup failed: %s", e)
|
||||
|
||||
def _slack_ignored_channels(self) -> set[str]:
|
||||
"""Configured Slack channels the generic gateway must never touch."""
|
||||
raw = self.config.extra.get("ignored_channels")
|
||||
if raw is None:
|
||||
raw = os.getenv("SLACK_IGNORED_CHANNELS")
|
||||
if raw is None:
|
||||
return set()
|
||||
if isinstance(raw, list):
|
||||
return {str(part).strip() for part in raw if str(part).strip()}
|
||||
return {part.strip() for part in str(raw).split(",") if part.strip()}
|
||||
|
||||
def _is_ignored_channel(self, channel_id: str) -> bool:
|
||||
"""Return True when generic Slack gateway must stay silent here.
|
||||
|
||||
Most Slack call sites pass the parent channel ID directly, but some
|
||||
gateway/session paths carry a thread-scoped identifier like
|
||||
``C123:1712345678.000001``. Ignored-channel matching is channel-level,
|
||||
so normalize defensively before checking the configured blacklist.
|
||||
"""
|
||||
if not channel_id:
|
||||
return False
|
||||
parent_channel_id = str(channel_id).split(":", 1)[0]
|
||||
ignored = self._slack_ignored_channels()
|
||||
return "*" in ignored or parent_channel_id in ignored
|
||||
|
||||
async def send(
|
||||
self,
|
||||
chat_id: str,
|
||||
|
|
@ -2397,6 +2422,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
metadata: Optional[Dict[str, Any]] = None,
|
||||
) -> SendResult:
|
||||
"""Send a message to a Slack channel or DM."""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.warning(
|
||||
"[Slack] Suppressed outbound generic send to configured ignored channel %s",
|
||||
chat_id,
|
||||
)
|
||||
return SendResult(success=False, error="ignored_channel")
|
||||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
|
|
@ -2576,6 +2607,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
metadata: Optional[Dict[str, Any]] = None,
|
||||
) -> SendResult:
|
||||
"""Send a Slack ephemeral message visible only to one user."""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.warning(
|
||||
"[Slack] Suppressed outbound generic ephemeral notice to configured ignored channel %s",
|
||||
chat_id,
|
||||
)
|
||||
return SendResult(success=False, error="ignored_channel")
|
||||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
if not chat_id or not user_id:
|
||||
|
|
@ -2658,6 +2695,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
metadata: Optional[Dict[str, Any]] = None,
|
||||
) -> SendResult:
|
||||
"""Edit a previously sent Slack message."""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.warning(
|
||||
"[Slack] Suppressed message edit in configured ignored channel %s",
|
||||
chat_id,
|
||||
)
|
||||
return SendResult(success=False, error="ignored_channel")
|
||||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
try:
|
||||
|
|
@ -2789,6 +2832,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
Requires the assistant:write or chat:write scope.
|
||||
Auto-clears when the bot sends a reply to the thread.
|
||||
"""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.debug("[Slack] Suppressed typing/status in configured ignored channel %s", chat_id)
|
||||
return
|
||||
if not self._app:
|
||||
return
|
||||
|
||||
|
|
@ -2877,6 +2923,10 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
|
||||
async def stop_typing(self, chat_id: str, metadata=None) -> None:
|
||||
"""Clear the assistant thread status indicator."""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.debug("[Slack] Suppressed status clear in configured ignored channel %s", chat_id)
|
||||
self._active_status_threads.pop(chat_id, None)
|
||||
return
|
||||
if not self._app:
|
||||
return
|
||||
requested_thread_ts = ""
|
||||
|
|
@ -3095,6 +3145,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
metadata: Optional[Dict[str, Any]] = None,
|
||||
) -> SendResult:
|
||||
"""Upload a local file to Slack."""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.warning(
|
||||
"[Slack] Suppressed file upload in configured ignored channel %s",
|
||||
chat_id,
|
||||
)
|
||||
return SendResult(success=False, error="ignored_channel")
|
||||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
|
|
@ -3149,6 +3205,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
|
||||
The batch limit is 10 file uploads per call (Slack server-side cap).
|
||||
"""
|
||||
if self._is_ignored_channel(chat_id):
|
||||
logger.warning(
|
||||
"[Slack] Suppressed multi-image upload in configured ignored channel %s",
|
||||
chat_id,
|
||||
)
|
||||
return
|
||||
if not self._app:
|
||||
return
|
||||
if not images:
|
||||
|
|
@ -4871,6 +4933,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
this lifecycle event.
|
||||
"""
|
||||
channel_id = event.get("channel_id") or event.get("channel") or ""
|
||||
if self._is_ignored_channel(channel_id):
|
||||
logger.info("[Slack] Ignoring file_shared event in configured ignored channel %s", channel_id)
|
||||
return
|
||||
file_id = event.get("file_id") or (event.get("file") or {}).get("id") or ""
|
||||
if not channel_id or not file_id:
|
||||
return
|
||||
|
|
@ -5155,6 +5220,11 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
):
|
||||
return
|
||||
|
||||
channel_id = event.get("channel", "")
|
||||
if self._is_ignored_channel(channel_id):
|
||||
logger.info("[Slack] Ignoring message in configured ignored channel %s", channel_id)
|
||||
return
|
||||
|
||||
# Bot/app-authored message filtering (SLACK_ALLOW_BOTS / config
|
||||
# allow_bots):
|
||||
# "none" — ignore all bot/app-authored messages (default,
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ from gateway.platforms.base import (
|
|||
MessageType,
|
||||
SendResult,
|
||||
SUPPORTED_VIDEO_TYPES,
|
||||
SendResult,
|
||||
is_host_excluded_by_no_proxy,
|
||||
)
|
||||
|
||||
|
|
@ -107,6 +108,85 @@ def test_slack_mock_bootstrap_preserves_installed_packages():
|
|||
if PathFinder.find_spec("slack_sdk") is not None:
|
||||
assert isinstance(importlib.import_module("slack_sdk.errors"), ModuleType)
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# TestIgnoredChannelOutboundSuppression
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestIgnoredChannelOutboundSuppression:
|
||||
"""Ignored Slack channels must be a hard generic-gateway kill switch."""
|
||||
|
||||
def _ignored_adapter(self):
|
||||
config = PlatformConfig(
|
||||
enabled=True,
|
||||
token="***",
|
||||
extra={"ignored_channels": ["C_PRD"]},
|
||||
)
|
||||
adapter = SlackAdapter(config)
|
||||
adapter._app = MagicMock()
|
||||
adapter._app.client = AsyncMock()
|
||||
adapter._bot_user_id = "U_BOT"
|
||||
adapter._running = True
|
||||
return adapter
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_suppressed_for_ignored_channel(self):
|
||||
adapter = self._ignored_adapter()
|
||||
|
||||
result = await adapter.send("C_PRD", "Acknowledged", reply_to="123.456")
|
||||
|
||||
assert result.success is False
|
||||
assert result.error == "ignored_channel"
|
||||
adapter._app.client.chat_postMessage.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_private_notice_suppressed_for_ignored_channel(self):
|
||||
adapter = self._ignored_adapter()
|
||||
|
||||
result = await adapter.send_private_notice(
|
||||
"C_PRD", "U_USER", "No home channel is set", reply_to="123.456"
|
||||
)
|
||||
|
||||
assert result.success is False
|
||||
assert result.error == "ignored_channel"
|
||||
adapter._app.client.chat_postEphemeral.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_edit_status_and_media_paths_suppressed(self, tmp_path):
|
||||
adapter = self._ignored_adapter()
|
||||
adapter._active_status_threads["C_PRD"] = "123.456"
|
||||
file_path = tmp_path / "note.txt"
|
||||
file_path.write_text("secret")
|
||||
|
||||
edit = await adapter.edit_message("C_PRD", "123.999", "updated", finalize=True)
|
||||
await adapter.send_typing("C_PRD", {"thread_ts": "123.456"})
|
||||
await adapter.stop_typing("C_PRD")
|
||||
upload = await adapter._upload_file("C_PRD", str(file_path))
|
||||
await adapter.send_multiple_images("C_PRD", [("https://example.com/image.png", "alt")])
|
||||
|
||||
assert edit.success is False
|
||||
assert upload.success is False
|
||||
assert edit.error == "ignored_channel"
|
||||
assert upload.error == "ignored_channel"
|
||||
adapter._app.client.chat_update.assert_not_awaited()
|
||||
adapter._app.client.assistant_threads_setStatus.assert_not_awaited()
|
||||
adapter._app.client.files_upload_v2.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_inbound_message_suppressed_for_ignored_channel(self):
|
||||
adapter = self._ignored_adapter()
|
||||
adapter.handle_message = AsyncMock()
|
||||
|
||||
await adapter._handle_slack_message({
|
||||
"text": "<@U_BOT> review this",
|
||||
"user": "U_USER",
|
||||
"channel": "C_PRD",
|
||||
"channel_type": "channel",
|
||||
"ts": "123.456",
|
||||
})
|
||||
|
||||
adapter.handle_message.assert_not_awaited()
|
||||
|
||||
|
||||
async def _pending_for_fake_task():
|
||||
# Stay pending so done-callbacks attached by the adapter (which would
|
||||
|
|
|
|||
116
tests/gateway/test_slack_runner_ignored_channels.py
Normal file
116
tests/gateway/test_slack_runner_ignored_channels.py
Normal file
|
|
@ -0,0 +1,116 @@
|
|||
import pytest
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
from gateway.config import GatewayConfig, Platform, PlatformConfig
|
||||
from gateway.platforms.base import MessageEvent, MessageType, SendResult
|
||||
from gateway.run import (
|
||||
GatewayRunner,
|
||||
_is_slack_ignored_channel,
|
||||
_slack_ignored_channels_from_gateway_config,
|
||||
)
|
||||
from gateway.session import SessionSource
|
||||
|
||||
|
||||
def _config_with_slack_extra(extra=None):
|
||||
return GatewayConfig(
|
||||
platforms={
|
||||
Platform.SLACK: PlatformConfig(enabled=True, extra=extra or {}),
|
||||
}
|
||||
)
|
||||
|
||||
|
||||
def test_slack_ignored_channels_from_config_list():
|
||||
config = _config_with_slack_extra({"ignored_channels": ["C_PRD", " C_OTHER ", ""]})
|
||||
|
||||
assert _slack_ignored_channels_from_gateway_config(config) == {"C_PRD", "C_OTHER"}
|
||||
|
||||
|
||||
def test_slack_ignored_channel_matches_thread_scoped_chat_id():
|
||||
config = _config_with_slack_extra({"ignored_channels": "C_PRD"})
|
||||
|
||||
assert _is_slack_ignored_channel(config, "C_PRD")
|
||||
assert _is_slack_ignored_channel(config, "C_PRD:1782283787.899249")
|
||||
|
||||
|
||||
def test_slack_ignored_channel_supports_wildcard():
|
||||
config = _config_with_slack_extra({"ignored_channels": "*"})
|
||||
|
||||
assert _is_slack_ignored_channel(config, "C_ANY")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_drops_slack_ignored_channel_before_auth_hooks_and_sessions(monkeypatch):
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = _config_with_slack_extra({"ignored_channels": "C_PRD"})
|
||||
runner._startup_restore_in_progress = False
|
||||
|
||||
# If the guard regresses, _handle_message will proceed into hooks/auth/session
|
||||
# setup and one of these sentinels will fail the test.
|
||||
runner.session_store = object()
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.plugins.invoke_hook",
|
||||
lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("hook should not run")),
|
||||
)
|
||||
runner._is_user_authorized = lambda source: (_ for _ in ()).throw(AssertionError("auth should not run"))
|
||||
|
||||
event = MessageEvent(
|
||||
text="<@U_BOT> review this PRD",
|
||||
message_type=MessageType.TEXT,
|
||||
source=SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
user_id="U_USER",
|
||||
user_name="shubham",
|
||||
chat_id="C_PRD",
|
||||
chat_type="group",
|
||||
),
|
||||
)
|
||||
|
||||
assert await runner._handle_message(event) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_runner_drops_thread_scoped_slack_ignored_channel():
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = _config_with_slack_extra({"ignored_channels": "C_PRD"})
|
||||
runner._startup_restore_in_progress = False
|
||||
runner._is_user_authorized = lambda source: (_ for _ in ()).throw(AssertionError("auth should not run"))
|
||||
runner.session_store = None
|
||||
|
||||
event = MessageEvent(
|
||||
text="<@U_BOT> review this PRD",
|
||||
message_type=MessageType.TEXT,
|
||||
source=SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
user_id="U_USER",
|
||||
user_name="shubham",
|
||||
chat_id="C_PRD:1782283787.899249",
|
||||
chat_type="group",
|
||||
thread_id="1782283787.899249",
|
||||
),
|
||||
)
|
||||
|
||||
assert await runner._handle_message(event) is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_platform_notice_suppressed_for_slack_ignored_channel():
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = _config_with_slack_extra({"ignored_channels": "C_PRD"})
|
||||
adapter = type("Adapter", (), {})()
|
||||
adapter.send = AsyncMock(return_value=SendResult(success=True))
|
||||
adapter.send_private_notice = AsyncMock(return_value=SendResult(success=True))
|
||||
runner.adapters = {Platform.SLACK: adapter}
|
||||
runner._thread_metadata_for_source = lambda source: {"thread_id": source.thread_id}
|
||||
|
||||
source = SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
user_id="U_USER",
|
||||
chat_id="C_PRD",
|
||||
chat_type="group",
|
||||
thread_id="1782283787.899249",
|
||||
)
|
||||
|
||||
await runner._deliver_platform_notice(source, "No home channel is set for Slack")
|
||||
|
||||
adapter.send.assert_not_called()
|
||||
adapter.send_private_notice.assert_not_called()
|
||||
Loading…
Add table
Add a link
Reference in a new issue