mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-26 17:38:36 +00:00
fix(slack): unify DM-resolution caches and bound wave-2 caches (C16 policy)
Post-rebase consolidation over the merged C7/C16/C10 work: - _ensure_dm_conversation now records workspace ownership via _remember_channel_team and bounds _dm_conversation_cache (cap 5000) - module-level _slack_dm_cache (C7 standalone path) bounded oldest-first - _user_is_bot_cache (C10) bounded with _trim_oldest_dict_entries - caption-mode contract tests updated for the C7+C8 merged media path
This commit is contained in:
parent
527c68baa4
commit
b0358cf3c8
3 changed files with 108 additions and 15 deletions
|
|
@ -19,11 +19,12 @@ import time
|
|||
from dataclasses import dataclass, field
|
||||
from typing import Callable, Dict, Optional, Any, Tuple, List
|
||||
|
||||
import aiohttp
|
||||
|
||||
try:
|
||||
from slack_bolt.async_app import AsyncApp
|
||||
from slack_bolt.adapter.socket_mode.async_handler import AsyncSocketModeHandler
|
||||
from slack_sdk.web.async_client import AsyncWebClient
|
||||
import aiohttp
|
||||
|
||||
SLACK_AVAILABLE = True
|
||||
except ImportError:
|
||||
|
|
@ -746,6 +747,7 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
self._CHANNEL_TEAM_MAX = 10000
|
||||
# user target (team_id:user_id) → opened DM conversation ID (D...)
|
||||
self._dm_conversation_cache: Dict[str, str] = {}
|
||||
self._DM_CONVERSATION_CACHE_MAX = 5000
|
||||
# Dedup cache: prevents duplicate bot responses when Socket Mode
|
||||
# reconnects redeliver events (#4777). The TTL must outlast Slack's
|
||||
# worst-case reconnect-redelivery gap, not just a few seconds — the
|
||||
|
|
@ -2025,9 +2027,12 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
dm_id = ((response or {}).get("channel") or {}).get("id")
|
||||
if dm_id:
|
||||
self._dm_conversation_cache[cache_key] = dm_id
|
||||
self._trim_oldest_dict_entries(
|
||||
self._dm_conversation_cache, self._DM_CONVERSATION_CACHE_MAX
|
||||
)
|
||||
# DM belongs to the same workspace as the user target.
|
||||
if team_id:
|
||||
self._channel_team[dm_id] = team_id
|
||||
self._remember_channel_team(dm_id, team_id)
|
||||
return dm_id
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
|
|
@ -3228,7 +3233,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
or (isinstance(profile, dict) and profile.get("bot_id"))
|
||||
)
|
||||
self._user_is_bot_cache[cache_key] = is_bot
|
||||
|
||||
self._trim_oldest_dict_entries(
|
||||
self._user_is_bot_cache, self._USER_NAME_CACHE_MAX
|
||||
)
|
||||
# Populate the name cache from the same users.info response so the
|
||||
# later source construction does not need a second API lookup.
|
||||
name = (
|
||||
|
|
@ -7089,6 +7096,13 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
# Cache for Slack user ID -> DM conversation ID resolution in the standalone
|
||||
# send path. Keyed by "{token}:{user_id}" to support multi-workspace setups.
|
||||
_slack_dm_cache: Dict[str, str] = {}
|
||||
_SLACK_DM_CACHE_MAX = 5000
|
||||
|
||||
|
||||
def _trim_slack_dm_cache() -> None:
|
||||
"""Bound the module-level DM cache, oldest-insertion-first (C16 policy)."""
|
||||
while len(_slack_dm_cache) > _SLACK_DM_CACHE_MAX:
|
||||
_slack_dm_cache.pop(next(iter(_slack_dm_cache)))
|
||||
|
||||
|
||||
async def _resolve_slack_user_dm(token: str, user_id: str) -> Optional[str]:
|
||||
|
|
@ -7128,6 +7142,7 @@ async def _resolve_slack_user_dm(token: str, user_id: str) -> Optional[str]:
|
|||
if data.get("ok") and data.get("channel", {}).get("id"):
|
||||
channel_id = data["channel"]["id"]
|
||||
_slack_dm_cache[cache_key] = channel_id
|
||||
_trim_slack_dm_cache()
|
||||
return channel_id
|
||||
logger.warning(
|
||||
"[Slack] conversations.open failed for %s: %s",
|
||||
|
|
@ -7249,6 +7264,9 @@ async def _standalone_send(
|
|||
|
||||
# --- Media path: AsyncWebClient.files_upload_v2 (+ optional text) ---
|
||||
if media_files:
|
||||
# Function-local import: tests inject a fake slack_sdk via
|
||||
# sys.modules, and installs without slack_sdk get a clean error
|
||||
# instead of an ImportError at module load.
|
||||
try:
|
||||
from slack_sdk.web.async_client import AsyncWebClient as _AsyncWebClient
|
||||
except ImportError:
|
||||
|
|
@ -7260,6 +7278,7 @@ async def _standalone_send(
|
|||
}
|
||||
|
||||
client = _AsyncWebClient(token=token)
|
||||
_apply_slack_proxy(client, resolve_proxy_url())
|
||||
last_message_id = None
|
||||
|
||||
# Caption mode: skip a separate text post; comment rides the upload.
|
||||
|
|
|
|||
|
|
@ -1221,25 +1221,63 @@ class TestSlackProxyBehavior:
|
|||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
from contextlib import contextmanager
|
||||
from types import ModuleType
|
||||
|
||||
|
||||
@contextmanager
|
||||
def _fake_slack_sdk_modules(client):
|
||||
"""Route ``from slack_sdk.web.async_client import AsyncWebClient`` to a mock."""
|
||||
import sys as _sys
|
||||
|
||||
sdk = ModuleType("slack_sdk")
|
||||
web = ModuleType("slack_sdk.web")
|
||||
async_client = ModuleType("slack_sdk.web.async_client")
|
||||
async_client.AsyncWebClient = MagicMock(return_value=client)
|
||||
sdk.web = web
|
||||
web.async_client = async_client
|
||||
modules = {
|
||||
"slack_sdk": sdk,
|
||||
"slack_sdk.web": web,
|
||||
"slack_sdk.web.async_client": async_client,
|
||||
}
|
||||
old = {name: _sys.modules.get(name) for name in modules}
|
||||
_sys.modules.update(modules)
|
||||
try:
|
||||
yield
|
||||
finally:
|
||||
for name, prev in old.items():
|
||||
if prev is None:
|
||||
_sys.modules.pop(name, None)
|
||||
else:
|
||||
_sys.modules[name] = prev
|
||||
|
||||
|
||||
class TestStandaloneSendMedia:
|
||||
@pytest.mark.asyncio
|
||||
async def test_uploads_local_media_with_message_as_caption(self, tmp_path):
|
||||
"""Standalone cron sends should use files_upload_v2, not omit the image."""
|
||||
"""Standalone cron sends must upload via files_upload_v2 — the text
|
||||
posts as its own message and the file follows (caption-mode, where
|
||||
text rides the upload as initial_comment, is chosen by the tool
|
||||
layer via the ``caption=`` kwarg — covered in
|
||||
tests/tools/test_slack_send_message_media.py)."""
|
||||
image = tmp_path / "daily-report.png"
|
||||
image.write_bytes(b"\x89PNG\r\n\x1a\n")
|
||||
client = MagicMock()
|
||||
client.chat_postMessage = AsyncMock(return_value={"ok": True, "ts": "1.0"})
|
||||
client.files_upload_v2 = AsyncMock(
|
||||
return_value={"ok": True, "files": [{"id": "F123"}]}
|
||||
)
|
||||
config = PlatformConfig(enabled=True, token="xoxb-fake-token")
|
||||
|
||||
with (
|
||||
patch.object(_slack_mod, "AsyncWebClient", return_value=client),
|
||||
_fake_slack_sdk_modules(client),
|
||||
patch.object(_slack_mod, "resolve_proxy_url", return_value=None),
|
||||
patch.object(
|
||||
_slack_mod.aiohttp,
|
||||
"ClientSession",
|
||||
side_effect=AssertionError("media delivery used text-only chat.postMessage"),
|
||||
side_effect=AssertionError("media delivery used text-only aiohttp path"),
|
||||
),
|
||||
):
|
||||
result = await _slack_mod._standalone_send(
|
||||
|
|
@ -1251,12 +1289,45 @@ class TestStandaloneSendMedia:
|
|||
)
|
||||
|
||||
assert result["success"] is True
|
||||
client.files_upload_v2.assert_awaited_once_with(
|
||||
channel="C123",
|
||||
file=str(image),
|
||||
filename="daily-report.png",
|
||||
initial_comment="daily report",
|
||||
thread_ts=None,
|
||||
client.chat_postMessage.assert_awaited_once()
|
||||
assert client.chat_postMessage.await_args.kwargs["text"] == "daily report"
|
||||
client.files_upload_v2.assert_awaited_once()
|
||||
up_kwargs = client.files_upload_v2.await_args.kwargs
|
||||
assert up_kwargs["channel"] == "C123"
|
||||
assert up_kwargs["file"] == str(image)
|
||||
assert up_kwargs["filename"] == "daily-report.png"
|
||||
assert up_kwargs["initial_comment"] == ""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_caption_kwarg_rides_upload_as_initial_comment(self, tmp_path):
|
||||
"""When the tool layer passes caption=, it rides the upload and no
|
||||
separate text message is posted (C8 caption-mode contract)."""
|
||||
image = tmp_path / "chart.png"
|
||||
image.write_bytes(b"\x89PNG\r\n\x1a\n")
|
||||
client = MagicMock()
|
||||
client.chat_postMessage = AsyncMock(return_value={"ok": True, "ts": "1.0"})
|
||||
client.files_upload_v2 = AsyncMock(
|
||||
return_value={"ok": True, "files": [{"id": "F123"}]}
|
||||
)
|
||||
config = PlatformConfig(enabled=True, token="xoxb-fake-token")
|
||||
|
||||
with (
|
||||
_fake_slack_sdk_modules(client),
|
||||
patch.object(_slack_mod, "resolve_proxy_url", return_value=None),
|
||||
):
|
||||
result = await _slack_mod._standalone_send(
|
||||
config,
|
||||
"C123",
|
||||
"",
|
||||
thread_id=None,
|
||||
media_files=[(str(image), False)],
|
||||
caption="Q3 chart",
|
||||
)
|
||||
|
||||
assert result["success"] is True
|
||||
client.chat_postMessage.assert_not_awaited()
|
||||
assert (
|
||||
client.files_upload_v2.await_args.kwargs["initial_comment"] == "Q3 chart"
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -1371,12 +1442,13 @@ class TestStandaloneSendUserDmResolution:
|
|||
open_resp = self._mock_resp({"ok": True, "channel": {"id": "D777666555"}})
|
||||
session = self._mock_session(open_resp)
|
||||
client = MagicMock()
|
||||
client.chat_postMessage = AsyncMock(return_value={"ok": True, "ts": "1.0"})
|
||||
client.files_upload_v2 = AsyncMock(return_value={"ok": True, "ts": "9.9"})
|
||||
config = PlatformConfig(enabled=True, token="xoxb-fake-token")
|
||||
|
||||
with (
|
||||
patch.object(_slack_mod.aiohttp, "ClientSession", return_value=session),
|
||||
patch.object(_slack_mod, "AsyncWebClient", return_value=client),
|
||||
_fake_slack_sdk_modules(client),
|
||||
patch.object(_slack_mod, "resolve_proxy_url", return_value=None),
|
||||
):
|
||||
result = await _slack_mod._standalone_send(
|
||||
|
|
|
|||
|
|
@ -785,13 +785,15 @@ class TestSendToPlatformChunking:
|
|||
entry.standalone_sender_fn = original
|
||||
|
||||
assert result["success"] is True
|
||||
# C8 caption-mode: short text rides the upload as `caption=` and the
|
||||
# message slot is emptied — one Slack bubble, not text + bare file.
|
||||
send.assert_awaited_once_with(
|
||||
pconfig,
|
||||
"C123",
|
||||
"daily report",
|
||||
"",
|
||||
thread_id=None,
|
||||
media_files=media_files,
|
||||
force_document=False,
|
||||
caption="daily report",
|
||||
)
|
||||
|
||||
def test_slack_bold_italic_formatted_before_send(self, monkeypatch):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue