mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-27 17:58:07 +00:00
fix(discord): deliver MEDIA video attachments instead of silent drop
Outbound MEDIA video/document tags on the Discord non-streaming path were extracted (stripped from the visible text) but never delivered: no attachment, no error, and no dispatch log line (#66797). The open-handle discord.File plus singular file= form could race Discord's multipart encoder after an earlier image batch on the same channel and return a successful message carrying zero attachments. Fix _send_file_attachment (used by send_video/send_document/ send_image_file) to: - use a path-based discord.File via the plural files=[...] kwarg, the same pattern as the working send_multiple_images batch path; - pre-flight os.path.isfile and return a File not found result instead of raising deep inside discord.File; - fail loud when Discord accepts the message but attaches nothing, so the dispatch loop surfaces a warning rather than a silent drop; - add INFO dispatch logs (base non-image MEDIA fan-out, video send, and file attachment) so a MEDIA:.mp4 cannot vanish without a trace. Tests (tests/gateway/test_discord_send.py): - path-based files=[...] kwarg used, singular file= unset; - fail-loud when the returned message has no attachments; - missing file fails fast without resolving the channel; - forum-parent delivery routes through create_thread with files=[...]; - end-to-end image+video response routes the mp4 to send_video while images still batch. Fixes #66797
This commit is contained in:
Piyush Jagadish Bag
Teknium
parent
b129a72e0d
commit
8eb29a1bb9
3 changed files with 258 additions and 10 deletions
|
|
@ -5460,6 +5460,12 @@ class BasePlatformAdapter(ABC):
|
|||
except Exception as batch_err:
|
||||
logger.warning("[%s] Error batching images: %s", self.name, batch_err, exc_info=True)
|
||||
|
||||
if _non_image_media:
|
||||
logger.info(
|
||||
"[%s] Delivering %d non-image MEDIA attachment(s)",
|
||||
self.name,
|
||||
len(_non_image_media),
|
||||
)
|
||||
for media_path, is_voice in _non_image_media:
|
||||
if human_delay > 0:
|
||||
await asyncio.sleep(human_delay)
|
||||
|
|
@ -5472,6 +5478,12 @@ class BasePlatformAdapter(ABC):
|
|||
metadata=_final_thread_metadata,
|
||||
)
|
||||
elif ext in _VIDEO_EXTS:
|
||||
logger.info(
|
||||
"[%s] Sending video attachment (%s) to %s",
|
||||
self.name,
|
||||
ext,
|
||||
event.source.chat_id,
|
||||
)
|
||||
media_result = await self.send_video(
|
||||
chat_id=event.source.chat_id,
|
||||
video_path=media_path,
|
||||
|
|
|
|||
|
|
@ -3341,10 +3341,20 @@ class DiscordAdapter(BasePlatformAdapter):
|
|||
|
||||
Forum channels (type 15) get a new thread whose starter message
|
||||
carries the file — they reject direct POST /messages.
|
||||
|
||||
Uses a path-based ``discord.File`` (same pattern as
|
||||
``send_multiple_images``) rather than an open file handle. The
|
||||
handle form can race with Discord's multipart encoder after an
|
||||
earlier image batch on the same channel and produce a successful
|
||||
message with zero attachments — a silent drop for video/document
|
||||
MEDIA tags (#66797).
|
||||
"""
|
||||
if not self._client:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
if not os.path.isfile(file_path):
|
||||
return SendResult(success=False, error=f"File not found: {file_path}")
|
||||
|
||||
channel = self._client.get_channel(int(chat_id))
|
||||
if not channel:
|
||||
channel = await self._client.fetch_channel(int(chat_id))
|
||||
|
|
@ -3352,15 +3362,45 @@ class DiscordAdapter(BasePlatformAdapter):
|
|||
return SendResult(success=False, error=f"Channel {chat_id} not found")
|
||||
|
||||
filename = file_name or os.path.basename(file_path)
|
||||
with open(file_path, "rb") as fh:
|
||||
file = discord.File(fh, filename=filename)
|
||||
if self._is_forum_parent(channel):
|
||||
return await self._forum_post_file(
|
||||
channel,
|
||||
content=(caption or "").strip(),
|
||||
file=file,
|
||||
)
|
||||
msg = await channel.send(content=caption if caption else None, file=file)
|
||||
logger.info(
|
||||
"[%s] Sending file attachment %s (%s) to %s",
|
||||
self.name,
|
||||
filename,
|
||||
os.path.splitext(filename)[1].lower() or "no-ext",
|
||||
chat_id,
|
||||
)
|
||||
# Path-based File: discord.py owns open/close for the upload, matching
|
||||
# the working image-batch path. Prefer ``files=[...]`` over deprecated
|
||||
# singular ``file=`` for the same reason.
|
||||
discord_file = discord.File(file_path, filename=filename)
|
||||
if self._is_forum_parent(channel):
|
||||
result = await self._forum_post_file(
|
||||
channel,
|
||||
content=(caption or "").strip(),
|
||||
files=[discord_file],
|
||||
)
|
||||
return result
|
||||
msg = await channel.send(
|
||||
content=caption if caption else None,
|
||||
files=[discord_file],
|
||||
)
|
||||
attachments = getattr(msg, "attachments", None) or []
|
||||
if not attachments:
|
||||
# Discord accepted the message but attached nothing — the failure
|
||||
# mode reported in #66797 (MEDIA video stripped from text, no
|
||||
# attachment, no prior log line). Fail loud so the dispatch loop
|
||||
# surfaces a warning instead of a silent drop.
|
||||
logger.warning(
|
||||
"[%s] Discord returned message %s with no attachments for %s",
|
||||
self.name,
|
||||
getattr(msg, "id", "?"),
|
||||
filename,
|
||||
)
|
||||
return SendResult(
|
||||
success=False,
|
||||
error=f"Discord accepted the message but attached no files ({filename})",
|
||||
message_id=str(getattr(msg, "id", "") or "") or None,
|
||||
)
|
||||
return SendResult(success=True, message_id=str(msg.id))
|
||||
|
||||
async def send_multiple_images(
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
import asyncio
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
|
|
@ -445,3 +446,198 @@ async def test_typing_stop_cleans_up():
|
|||
|
||||
await adapter.stop_typing("12345")
|
||||
assert "12345" not in adapter._typing_tasks
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #66797 — outbound MEDIA video must reach channel.send as a real attachment
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_uses_path_based_files_kwarg(tmp_path, monkeypatch):
|
||||
"""Regression for #66797: video MEDIA delivery must use path-based
|
||||
``discord.File`` via ``files=[...]`` (same pattern as image batching).
|
||||
|
||||
The previous open-handle + singular ``file=`` form could return a successful
|
||||
message with zero attachments after an earlier image batch on the same
|
||||
channel — silent drop from the user's perspective.
|
||||
"""
|
||||
import plugins.platforms.discord.adapter as discord_platform
|
||||
|
||||
video = tmp_path / "clip.mp4"
|
||||
video.write_bytes(b"\x00\x00\x00\x18ftypmp42fake")
|
||||
|
||||
captured = {}
|
||||
|
||||
class _FakeFile:
|
||||
def __init__(self, fp, filename=None, **kwargs):
|
||||
captured["fp"] = fp
|
||||
captured["filename"] = filename
|
||||
|
||||
monkeypatch.setattr(discord_platform.discord, "File", _FakeFile)
|
||||
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="***"))
|
||||
sent_msg = SimpleNamespace(
|
||||
id=4242,
|
||||
attachments=[SimpleNamespace(filename="clip.mp4", url="https://cdn.example/clip.mp4")],
|
||||
)
|
||||
channel = SimpleNamespace(
|
||||
send=AsyncMock(return_value=sent_msg),
|
||||
type=0,
|
||||
)
|
||||
adapter._client = SimpleNamespace(
|
||||
get_channel=lambda _chat_id: channel,
|
||||
fetch_channel=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(adapter, "_is_forum_parent", lambda _ch: False)
|
||||
|
||||
result = await adapter.send_video("555", str(video))
|
||||
|
||||
assert result.success is True
|
||||
assert result.message_id == "4242"
|
||||
assert captured["fp"] == str(video)
|
||||
assert captured["filename"] == "clip.mp4"
|
||||
channel.send.assert_awaited_once()
|
||||
send_kwargs = channel.send.await_args.kwargs
|
||||
assert send_kwargs.get("file") is None
|
||||
assert isinstance(send_kwargs.get("files"), list) and len(send_kwargs["files"]) == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_fails_loud_when_message_has_no_attachments(tmp_path, monkeypatch):
|
||||
"""If Discord accepts the message but attaches nothing, fail loud (#66797)."""
|
||||
import plugins.platforms.discord.adapter as discord_platform
|
||||
|
||||
video = tmp_path / "clip.mp4"
|
||||
video.write_bytes(b"fake-mp4")
|
||||
|
||||
monkeypatch.setattr(
|
||||
discord_platform.discord,
|
||||
"File",
|
||||
lambda fp, filename=None, **kwargs: SimpleNamespace(fp=fp, filename=filename),
|
||||
)
|
||||
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="***"))
|
||||
# Message id present, but no attachments — the silent-drop failure mode.
|
||||
sent_msg = SimpleNamespace(id=99, attachments=[])
|
||||
channel = SimpleNamespace(send=AsyncMock(return_value=sent_msg), type=0)
|
||||
adapter._client = SimpleNamespace(
|
||||
get_channel=lambda _chat_id: channel,
|
||||
fetch_channel=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(adapter, "_is_forum_parent", lambda _ch: False)
|
||||
|
||||
result = await adapter.send_video("555", str(video))
|
||||
|
||||
assert result.success is False
|
||||
assert "no files" in (result.error or "").lower()
|
||||
channel.send.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_deliver_media_from_response_routes_mp4_to_send_video(tmp_path, monkeypatch):
|
||||
"""Streaming/post-stream dispatch must call send_video for MEDIA:.mp4."""
|
||||
from gateway.platforms.base import BasePlatformAdapter, SendResult
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
video = tmp_path / "clip.mp4"
|
||||
video.write_bytes(b"fake-mp4")
|
||||
image = tmp_path / "figure.png"
|
||||
image.write_bytes(b"fake-png")
|
||||
|
||||
# Allow delivery from tmp_path in non-strict mode (default).
|
||||
monkeypatch.chdir(tmp_path)
|
||||
|
||||
adapter = SimpleNamespace(
|
||||
name="Discord",
|
||||
extract_media=BasePlatformAdapter.extract_media,
|
||||
extract_images=BasePlatformAdapter.extract_images,
|
||||
extract_local_files=BasePlatformAdapter.extract_local_files,
|
||||
send_voice=AsyncMock(return_value=SendResult(success=True, message_id="v")),
|
||||
send_document=AsyncMock(return_value=SendResult(success=True, message_id="d")),
|
||||
send_image_file=AsyncMock(return_value=SendResult(success=True, message_id="i")),
|
||||
send_video=AsyncMock(return_value=SendResult(success=True, message_id="vid")),
|
||||
send_multiple_images=AsyncMock(),
|
||||
)
|
||||
event = SimpleNamespace(
|
||||
source=SimpleNamespace(
|
||||
platform="discord",
|
||||
chat_id="chat-1",
|
||||
thread_id=None,
|
||||
)
|
||||
)
|
||||
runner = SimpleNamespace(
|
||||
_thread_metadata_for_source=lambda source, anchor=None: {},
|
||||
_reply_anchor_for_event=lambda event: None,
|
||||
)
|
||||
response = (
|
||||
f"Here is the figure:\n\nMEDIA:{image}\n\n"
|
||||
f"And the clip:\n\nMEDIA:{video}\n"
|
||||
)
|
||||
|
||||
await GatewayRunner._deliver_media_from_response(runner, response, event, adapter)
|
||||
|
||||
adapter.send_video.assert_awaited_once()
|
||||
sent_path = adapter.send_video.await_args.kwargs["video_path"]
|
||||
assert Path(sent_path).resolve() == video.resolve()
|
||||
adapter.send_multiple_images.assert_awaited_once()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_missing_file_fails_fast_without_touching_channel():
|
||||
"""A missing MEDIA path must fail loud before any Discord I/O (#66797).
|
||||
|
||||
The pre-flight ``os.path.isfile`` guard turns a would-be crash inside
|
||||
``discord.File`` into an actionable ``File not found`` result, and must
|
||||
short-circuit before the channel is ever resolved.
|
||||
"""
|
||||
def _boom(*_args, **_kwargs):
|
||||
raise AssertionError("channel must not be resolved for a missing file")
|
||||
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="***"))
|
||||
adapter._client = SimpleNamespace(get_channel=_boom, fetch_channel=AsyncMock(side_effect=_boom))
|
||||
|
||||
result = await adapter.send_video("555", "/no/such/clip.mp4")
|
||||
|
||||
assert result.success is False
|
||||
assert "not found" in (result.error or "").lower()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_file_attachment_forum_uses_files_kwarg(tmp_path, monkeypatch):
|
||||
"""Forum-parent delivery must also route the path-based file through the
|
||||
plural ``files=[...]`` kwarg (#66797), so the create_thread starter message
|
||||
carries the attachment rather than silently dropping it."""
|
||||
import plugins.platforms.discord.adapter as discord_platform
|
||||
|
||||
video = tmp_path / "clip.mp4"
|
||||
video.write_bytes(b"fake-mp4")
|
||||
|
||||
monkeypatch.setattr(
|
||||
discord_platform.discord,
|
||||
"File",
|
||||
lambda fp, filename=None, **kwargs: SimpleNamespace(fp=fp, filename=filename),
|
||||
)
|
||||
|
||||
adapter = DiscordAdapter(PlatformConfig(enabled=True, token="***"))
|
||||
created_thread = SimpleNamespace(
|
||||
id=7,
|
||||
message=SimpleNamespace(id=8),
|
||||
)
|
||||
forum_channel = SimpleNamespace(
|
||||
id=7,
|
||||
create_thread=AsyncMock(return_value=created_thread),
|
||||
)
|
||||
adapter._client = SimpleNamespace(
|
||||
get_channel=lambda _chat_id: forum_channel,
|
||||
fetch_channel=AsyncMock(),
|
||||
)
|
||||
monkeypatch.setattr(adapter, "_is_forum_parent", lambda _ch: True)
|
||||
|
||||
result = await adapter.send_video("555", str(video))
|
||||
|
||||
assert result.success is True
|
||||
forum_channel.create_thread.assert_awaited_once()
|
||||
thread_kwargs = forum_channel.create_thread.await_args.kwargs
|
||||
assert thread_kwargs.get("file") is None
|
||||
assert isinstance(thread_kwargs.get("files"), list) and len(thread_kwargs["files"]) == 1
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue