mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-23 16:36:23 +00:00
fix(slack): never silently drop or truncate slash replies (#19688)
Two silent-loss modes in the ephemeral slash reply path: 1. Delivery failure was swallowed: _send_slash_ephemeral returned success=True on any POST failure, so the user's actual command reply vanished behind the stale 'Running /cmd…' ack. It now returns success=False and send() falls back to normal channel delivery. 2. Long replies were truncated to the first ~39k chunk with no notice. Replies are now chunked across response_url POSTs (first replaces the ack, follow-ups append, all ephemeral), capped at Slack's 5-POST response_url budget with an explicit truncation notice when exceeded. Also updates the #55357 bounded-error-read test for the #26788 precise context matching (ContextVar must be set) and channel fallback. Fixes #19688
This commit is contained in:
Teknium
parent
c2d306c4c2
commit
507a133e07
2 changed files with 186 additions and 62 deletions
|
|
@ -1180,44 +1180,66 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
lets us swap the "Running /cmd…" placeholder with the real reply,
|
||||
and the message stays ephemeral ("Only visible to you").
|
||||
|
||||
Falls back to a simple ``True`` SendResult if the POST fails —
|
||||
the user already saw the initial ack, so a delivery failure here
|
||||
is non-critical.
|
||||
Long replies are chunked: the first chunk replaces the ack, the
|
||||
rest are posted as additional ephemeral messages. Slack allows at
|
||||
most 5 POSTs to a response_url, so anything beyond that is closed
|
||||
with an explicit truncation notice instead of being silently
|
||||
dropped (#19688).
|
||||
|
||||
Returns ``success=False`` on delivery failure so the caller
|
||||
(``send()``) can fall back to normal channel delivery — the reply
|
||||
must never be silently dropped just because the ephemeral swap
|
||||
failed (#19688).
|
||||
"""
|
||||
formatted = self.format_message(content)
|
||||
# Slack's response_url has the same ~40k char limit as chat_postMessage.
|
||||
# Truncate to MAX_MESSAGE_LENGTH and use only the first chunk — the
|
||||
# response_url replaces a single ephemeral ack, so multi-chunk isn't
|
||||
# possible. Long responses are rare for command replies.
|
||||
chunks = self.truncate_message(formatted, self.MAX_MESSAGE_LENGTH)
|
||||
text = chunks[0] if chunks else formatted
|
||||
payload = {
|
||||
"response_type": "ephemeral",
|
||||
"replace_original": True,
|
||||
"text": text,
|
||||
}
|
||||
if not chunks:
|
||||
chunks = [formatted]
|
||||
# Slack allows at most 5 POSTs per response_url. Reserve the flow:
|
||||
# 1 replace + up to 4 follow-ups; announce anything left over.
|
||||
_MAX_RESPONSE_URL_POSTS = 5
|
||||
if len(chunks) > _MAX_RESPONSE_URL_POSTS:
|
||||
dropped = len(chunks) - _MAX_RESPONSE_URL_POSTS
|
||||
chunks = chunks[:_MAX_RESPONSE_URL_POSTS]
|
||||
chunks[-1] = (
|
||||
chunks[-1].rstrip()
|
||||
+ f"\n\n_[Reply truncated: {dropped} more part(s) exceeded "
|
||||
"Slack's ephemeral reply limit.]_"
|
||||
)
|
||||
try:
|
||||
async with aiohttp.ClientSession(trust_env=True) as session:
|
||||
async with session.post(
|
||||
ctx["response_url"],
|
||||
json=payload,
|
||||
timeout=aiohttp.ClientTimeout(total=10),
|
||||
) as resp:
|
||||
if resp.status == 200:
|
||||
return SendResult(success=True, message_id=None)
|
||||
body = await _read_error_text_limited(resp)
|
||||
logger.warning(
|
||||
"[Slack] response_url POST returned %s: %s",
|
||||
resp.status,
|
||||
body[:200],
|
||||
)
|
||||
for idx, chunk in enumerate(chunks):
|
||||
payload = {
|
||||
"response_type": "ephemeral",
|
||||
# Only the first chunk replaces the "Running /cmd…"
|
||||
# ack; the rest append as new ephemeral messages.
|
||||
"replace_original": idx == 0,
|
||||
"text": chunk,
|
||||
}
|
||||
async with session.post(
|
||||
ctx["response_url"],
|
||||
json=payload,
|
||||
timeout=aiohttp.ClientTimeout(total=10),
|
||||
) as resp:
|
||||
if resp.status != 200:
|
||||
body = await _read_error_text_limited(resp)
|
||||
logger.warning(
|
||||
"[Slack] response_url POST returned %s: %s",
|
||||
resp.status,
|
||||
body[:200],
|
||||
)
|
||||
return SendResult(
|
||||
success=False,
|
||||
error=f"response_url POST returned {resp.status}",
|
||||
)
|
||||
return SendResult(success=True, message_id=None)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"[Slack] response_url POST failed: %s",
|
||||
e,
|
||||
)
|
||||
# Non-fatal — the user saw the initial ack already.
|
||||
return SendResult(success=True, message_id=None)
|
||||
return SendResult(success=False, error=str(e))
|
||||
|
||||
def _warn_if_missing_group_dm_scopes(self, auth_response, team_name: str) -> None:
|
||||
"""Nudge existing installs to reinstall when group-DM scopes are absent.
|
||||
|
|
@ -1825,10 +1847,21 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
# the actual command reply ephemerally instead of posting publicly.
|
||||
slash_ctx = self._pop_slash_context(chat_id)
|
||||
if slash_ctx:
|
||||
return await self._send_slash_ephemeral(
|
||||
ephemeral_result = await self._send_slash_ephemeral(
|
||||
slash_ctx,
|
||||
content,
|
||||
)
|
||||
if ephemeral_result.success:
|
||||
return ephemeral_result
|
||||
# Ephemeral delivery failed (#19688): fall through to normal
|
||||
# channel delivery so the command reply is never silently
|
||||
# dropped. The stale "Running /cmd…" ack remains, but the
|
||||
# user still gets the actual answer.
|
||||
logger.warning(
|
||||
"[Slack] Ephemeral slash reply failed (%s); falling back "
|
||||
"to channel delivery",
|
||||
ephemeral_result.error,
|
||||
)
|
||||
|
||||
# Convert standard markdown → Slack mrkdwn
|
||||
formatted = self.format_message(content)
|
||||
|
|
|
|||
|
|
@ -5515,7 +5515,7 @@ class TestSlashEphemeralAck:
|
|||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_fallback_on_post_failure(self, adapter):
|
||||
"""_send_slash_ephemeral returns success=True even if POST fails."""
|
||||
"""Failed response_url POST falls back to normal channel delivery (#19688)."""
|
||||
import time
|
||||
from plugins.platforms.slack.adapter import _slash_user_id
|
||||
|
||||
|
|
@ -5536,6 +5536,10 @@ class TestSlashEphemeralAck:
|
|||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ts": "1234.5678", "ok": True}
|
||||
)
|
||||
|
||||
token = _slash_user_id.set("U1")
|
||||
try:
|
||||
with patch(
|
||||
|
|
@ -5545,8 +5549,112 @@ class TestSlashEphemeralAck:
|
|||
finally:
|
||||
_slash_user_id.reset(token)
|
||||
|
||||
# Still success — the user saw the initial ack already
|
||||
# Reply must not be silently dropped: channel fallback delivered it.
|
||||
assert result.success is True
|
||||
adapter._app.client.chat_postMessage.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_fallback_on_exception(self, adapter):
|
||||
"""aiohttp exception on response_url falls back to channel delivery (#19688)."""
|
||||
import time
|
||||
from plugins.platforms.slack.adapter import _slash_user_id
|
||||
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/timeout",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(side_effect=Exception("connection timeout"))
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ts": "1234.5678", "ok": True}
|
||||
)
|
||||
|
||||
token = _slash_user_id.set("U1")
|
||||
try:
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session
|
||||
):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
finally:
|
||||
_slash_user_id.reset(token)
|
||||
|
||||
assert result.success is True
|
||||
adapter._app.client.chat_postMessage.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_multichunk_delivers_all_parts(self, adapter):
|
||||
"""Long slash replies post every chunk instead of dropping the tail (#19688)."""
|
||||
import time
|
||||
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/long",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_resp = AsyncMock()
|
||||
mock_resp.status = 200
|
||||
mock_resp.__aenter__ = AsyncMock(return_value=mock_resp)
|
||||
mock_resp.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(return_value=mock_resp)
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
long_content = "A" * (adapter.MAX_MESSAGE_LENGTH + 500)
|
||||
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session
|
||||
):
|
||||
result = await adapter._send_slash_ephemeral(
|
||||
{"response_url": "https://hooks.slack.com/commands/long"},
|
||||
long_content,
|
||||
)
|
||||
|
||||
assert result.success is True
|
||||
assert mock_session.post.call_count >= 2
|
||||
# First POST replaces the ack; follow-ups append.
|
||||
first_payload = mock_session.post.call_args_list[0][1]["json"]
|
||||
second_payload = mock_session.post.call_args_list[1][1]["json"]
|
||||
assert first_payload["replace_original"] is True
|
||||
assert second_payload["replace_original"] is False
|
||||
# No content byte is lost.
|
||||
total_text = "".join(
|
||||
c[1]["json"]["text"] for c in mock_session.post.call_args_list
|
||||
)
|
||||
assert total_text.count("A") == len(long_content)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_caps_posts_with_truncation_notice(self, adapter):
|
||||
"""Beyond Slack's 5-POST response_url budget, truncation is announced."""
|
||||
mock_resp = AsyncMock()
|
||||
mock_resp.status = 200
|
||||
mock_resp.__aenter__ = AsyncMock(return_value=mock_resp)
|
||||
mock_resp.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(return_value=mock_resp)
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
very_long = "B" * (adapter.MAX_MESSAGE_LENGTH * 7)
|
||||
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session
|
||||
):
|
||||
result = await adapter._send_slash_ephemeral(
|
||||
{"response_url": "https://hooks.slack.com/commands/huge"},
|
||||
very_long,
|
||||
)
|
||||
|
||||
assert result.success is True
|
||||
assert mock_session.post.call_count == 5
|
||||
last_text = mock_session.post.call_args_list[-1][1]["json"]["text"]
|
||||
assert "Reply truncated" in last_text
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_limits_error_body(self, adapter):
|
||||
|
|
@ -5601,11 +5709,21 @@ class TestSlashEphemeralAck:
|
|||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession",
|
||||
return_value=mock_session,
|
||||
):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ts": "1234.5678", "ok": True}
|
||||
)
|
||||
|
||||
from plugins.platforms.slack.adapter import _slash_user_id
|
||||
|
||||
token = _slash_user_id.set("U1")
|
||||
try:
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession",
|
||||
return_value=mock_session,
|
||||
):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
finally:
|
||||
_slash_user_id.reset(token)
|
||||
|
||||
assert result.success is True
|
||||
assert response.text_calls == 0
|
||||
|
|
@ -5615,33 +5733,6 @@ class TestSlashEphemeralAck:
|
|||
)
|
||||
assert response.released is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_slash_ephemeral_fallback_on_exception(self, adapter):
|
||||
"""_send_slash_ephemeral returns success=True even if aiohttp raises."""
|
||||
import time
|
||||
from plugins.platforms.slack.adapter import _slash_user_id
|
||||
|
||||
adapter._slash_command_contexts[("C1", "U1")] = {
|
||||
"response_url": "https://hooks.slack.com/commands/timeout",
|
||||
"ts": time.monotonic(),
|
||||
}
|
||||
|
||||
mock_session = AsyncMock()
|
||||
mock_session.post = MagicMock(side_effect=Exception("connection timeout"))
|
||||
mock_session.__aenter__ = AsyncMock(return_value=mock_session)
|
||||
mock_session.__aexit__ = AsyncMock(return_value=False)
|
||||
|
||||
token = _slash_user_id.set("U1")
|
||||
try:
|
||||
with patch(
|
||||
"plugins.platforms.slack.adapter.aiohttp.ClientSession", return_value=mock_session
|
||||
):
|
||||
result = await adapter.send("C1", "Some response")
|
||||
finally:
|
||||
_slash_user_id.reset(token)
|
||||
|
||||
assert result.success is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_native_slash_stashes_context_and_dispatches(self, adapter):
|
||||
"""Full flow: native /q slash → stash + handle_message dispatch."""
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue