fix(signal): implement send_image_file, send_voice, and send_video for MEDIA: tag delivery
The Signal adapter inherited base class defaults for send_image_file(), send_voice(), and send_video() which only sent the file path as text (e.g. '🖼️ Image: /tmp/chart.png') instead of actually delivering the file as a Signal attachment. When agent responses contain MEDIA:/path/to/file tags, the gateway media pipeline extracts them and routes through these methods by file type. Without proper overrides, image/audio/video files were never actually delivered to Signal users. Extract a shared _send_attachment() helper that handles all file validation, size checking, group/DM routing, and RPC dispatch. The four public methods (send_document, send_image_file, send_voice, send_video) now delegate to this helper, following the same pattern used by WhatsApp (_send_media_to_bridge) and Discord (_send_file_attachment). The helper also uses a single stat() call with try/except FileNotFoundError instead of the previous exists() + stat() two-syscall pattern, eliminating a TOCTOU race. As a bonus, send_document() now gains the 100MB size check that was previously missing (inconsistency with send_image). Add 25 tests covering all methods plus MEDIA: tag extraction integration, method-override guards, and send_document's new size check. Fixes #5105
This commit is contained in:
kshitijk4poor
@@ -717,19 +717,27 @@ class SignalAdapter(BasePlatformAdapter):
|
||||
return SendResult(success=True)
|
||||
return SendResult(success=False, error="RPC send with attachment failed")
|
||||
|
||||
async def send_document(
|
||||
async def _send_attachment(
|
||||
self,
|
||||
chat_id: str,
|
||||
file_path: str,
|
||||
media_label: str,
|
||||
caption: Optional[str] = None,
|
||||
filename: Optional[str] = None,
|
||||
**kwargs,
|
||||
) -> SendResult:
|
||||
"""Send a document/file attachment."""
|
||||
"""Send any file as a Signal attachment via RPC.
|
||||
|
||||
Shared implementation for send_document, send_image_file, send_voice,
|
||||
and send_video — avoids duplicating the validation/routing/RPC logic.
|
||||
"""
|
||||
await self._stop_typing_indicator(chat_id)
|
||||
|
||||
if not Path(file_path).exists():
|
||||
return SendResult(success=False, error="File not found")
|
||||
try:
|
||||
file_size = Path(file_path).stat().st_size
|
||||
except FileNotFoundError:
|
||||
return SendResult(success=False, error=f"{media_label} file not found: {file_path}")
|
||||
|
||||
if file_size > SIGNAL_MAX_ATTACHMENT_SIZE:
|
||||
return SendResult(success=False, error=f"{media_label} too large ({file_size} bytes)")
|
||||
|
||||
params: Dict[str, Any] = {
|
||||
"account": self.account,
|
||||
@@ -746,7 +754,59 @@ class SignalAdapter(BasePlatformAdapter):
|
||||
if result is not None:
|
||||
self._track_sent_timestamp(result)
|
||||
return SendResult(success=True)
|
||||
return SendResult(success=False, error="RPC send document failed")
|
||||
return SendResult(success=False, error=f"RPC send {media_label.lower()} failed")
|
||||
|
||||
async def send_document(
|
||||
self,
|
||||
chat_id: str,
|
||||
file_path: str,
|
||||
caption: Optional[str] = None,
|
||||
filename: Optional[str] = None,
|
||||
**kwargs,
|
||||
) -> SendResult:
|
||||
"""Send a document/file attachment."""
|
||||
return await self._send_attachment(chat_id, file_path, "File", caption)
|
||||
|
||||
async def send_image_file(
|
||||
self,
|
||||
chat_id: str,
|
||||
image_path: str,
|
||||
caption: Optional[str] = None,
|
||||
reply_to: Optional[str] = None,
|
||||
**kwargs,
|
||||
) -> SendResult:
|
||||
"""Send a local image file as a native Signal attachment.
|
||||
|
||||
Called by the gateway media delivery flow when MEDIA: tags containing
|
||||
image paths are extracted from agent responses.
|
||||
"""
|
||||
return await self._send_attachment(chat_id, image_path, "Image", caption)
|
||||
|
||||
async def send_voice(
|
||||
self,
|
||||
chat_id: str,
|
||||
audio_path: str,
|
||||
caption: Optional[str] = None,
|
||||
reply_to: Optional[str] = None,
|
||||
**kwargs,
|
||||
) -> SendResult:
|
||||
"""Send an audio file as a Signal attachment.
|
||||
|
||||
Signal does not distinguish voice messages from file attachments at
|
||||
the API level, so this routes through the same RPC send path.
|
||||
"""
|
||||
return await self._send_attachment(chat_id, audio_path, "Audio", caption)
|
||||
|
||||
async def send_video(
|
||||
self,
|
||||
chat_id: str,
|
||||
video_path: str,
|
||||
caption: Optional[str] = None,
|
||||
reply_to: Optional[str] = None,
|
||||
**kwargs,
|
||||
) -> SendResult:
|
||||
"""Send a video file as a Signal attachment."""
|
||||
return await self._send_attachment(chat_id, video_path, "Video", caption)
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# Typing Indicators
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
import base64
|
||||
import json
|
||||
import pytest
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch, AsyncMock
|
||||
from urllib.parse import quote
|
||||
|
||||
@@ -368,3 +369,341 @@ class TestSignalSendMessage:
|
||||
# Just verify the import works and Signal is a valid platform
|
||||
from gateway.config import Platform
|
||||
assert Platform.SIGNAL.value == "signal"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# send_image_file method (#5105)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSignalSendImageFile:
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_image_file_sends_via_rpc(self, monkeypatch, tmp_path):
|
||||
"""send_image_file should send image as attachment via signal-cli RPC."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, captured = _stub_rpc({"timestamp": 1234567890})
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
img_path = tmp_path / "chart.png"
|
||||
img_path.write_bytes(b"\x89PNG" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_image_file(chat_id="+155****4567", image_path=str(img_path))
|
||||
|
||||
assert result.success is True
|
||||
assert len(captured) == 1
|
||||
assert captured[0]["method"] == "send"
|
||||
assert captured[0]["params"]["account"] == adapter.account
|
||||
assert captured[0]["params"]["recipient"] == ["+155****4567"]
|
||||
assert captured[0]["params"]["attachments"] == [str(img_path)]
|
||||
assert captured[0]["params"]["message"] == "" # caption=None → ""
|
||||
# Typing indicator must be stopped before sending
|
||||
adapter._stop_typing_indicator.assert_awaited_once_with("+155****4567")
|
||||
# Timestamp must be tracked for echo-back prevention
|
||||
assert 1234567890 in adapter._recent_sent_timestamps
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_image_file_to_group(self, monkeypatch, tmp_path):
|
||||
"""send_image_file should route group chats via groupId."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, captured = _stub_rpc({"timestamp": 1234567890})
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
img_path = tmp_path / "photo.jpg"
|
||||
img_path.write_bytes(b"\xff\xd8" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_image_file(
|
||||
chat_id="group:abc123==", image_path=str(img_path), caption="Here's the chart"
|
||||
)
|
||||
|
||||
assert result.success is True
|
||||
assert captured[0]["params"]["groupId"] == "abc123=="
|
||||
assert captured[0]["params"]["message"] == "Here's the chart"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_image_file_missing(self, monkeypatch):
|
||||
"""send_image_file should fail gracefully for nonexistent files."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
result = await adapter.send_image_file(chat_id="+155****4567", image_path="/nonexistent.png")
|
||||
|
||||
assert result.success is False
|
||||
assert "not found" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_image_file_too_large(self, monkeypatch, tmp_path):
|
||||
"""send_image_file should reject files over 100MB."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
img_path = tmp_path / "huge.png"
|
||||
img_path.write_bytes(b"x")
|
||||
|
||||
def mock_stat(self, **kwargs):
|
||||
class FakeStat:
|
||||
st_size = 200 * 1024 * 1024 # 200 MB
|
||||
return FakeStat()
|
||||
|
||||
with patch.object(Path, "stat", mock_stat):
|
||||
result = await adapter.send_image_file(chat_id="+155****4567", image_path=str(img_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "too large" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_image_file_rpc_failure(self, monkeypatch, tmp_path):
|
||||
"""send_image_file should return error when RPC returns None."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, _ = _stub_rpc(None)
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
img_path = tmp_path / "test.png"
|
||||
img_path.write_bytes(b"\x89PNG" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_image_file(chat_id="+155****4567", image_path=str(img_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "failed" in result.error.lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# send_voice method (#5105)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSignalSendVoice:
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_voice_sends_via_rpc(self, monkeypatch, tmp_path):
|
||||
"""send_voice should send audio as attachment via signal-cli RPC."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, captured = _stub_rpc({"timestamp": 1234567890})
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
audio_path = tmp_path / "reply.ogg"
|
||||
audio_path.write_bytes(b"OggS" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_voice(chat_id="+155****4567", audio_path=str(audio_path))
|
||||
|
||||
assert result.success is True
|
||||
assert captured[0]["method"] == "send"
|
||||
assert captured[0]["params"]["attachments"] == [str(audio_path)]
|
||||
assert captured[0]["params"]["message"] == "" # caption=None → ""
|
||||
adapter._stop_typing_indicator.assert_awaited_once_with("+155****4567")
|
||||
assert 1234567890 in adapter._recent_sent_timestamps
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_voice_missing_file(self, monkeypatch):
|
||||
"""send_voice should fail for nonexistent audio."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
result = await adapter.send_voice(chat_id="+155****4567", audio_path="/missing.ogg")
|
||||
|
||||
assert result.success is False
|
||||
assert "not found" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_voice_to_group(self, monkeypatch, tmp_path):
|
||||
"""send_voice should route group chats correctly."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, captured = _stub_rpc({"timestamp": 9999})
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
audio_path = tmp_path / "note.mp3"
|
||||
audio_path.write_bytes(b"\xff\xe0" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_voice(chat_id="group:grp1==", audio_path=str(audio_path))
|
||||
|
||||
assert result.success is True
|
||||
assert captured[0]["params"]["groupId"] == "grp1=="
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_voice_too_large(self, monkeypatch, tmp_path):
|
||||
"""send_voice should reject files over 100MB."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
audio_path = tmp_path / "huge.ogg"
|
||||
audio_path.write_bytes(b"x")
|
||||
|
||||
def mock_stat(self, **kwargs):
|
||||
class FakeStat:
|
||||
st_size = 200 * 1024 * 1024
|
||||
return FakeStat()
|
||||
|
||||
with patch.object(Path, "stat", mock_stat):
|
||||
result = await adapter.send_voice(chat_id="+155****4567", audio_path=str(audio_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "too large" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_voice_rpc_failure(self, monkeypatch, tmp_path):
|
||||
"""send_voice should return error when RPC returns None."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, _ = _stub_rpc(None)
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
audio_path = tmp_path / "reply.ogg"
|
||||
audio_path.write_bytes(b"OggS" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_voice(chat_id="+155****4567", audio_path=str(audio_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "failed" in result.error.lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# send_video method (#5105)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSignalSendVideo:
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_sends_via_rpc(self, monkeypatch, tmp_path):
|
||||
"""send_video should send video as attachment via signal-cli RPC."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, captured = _stub_rpc({"timestamp": 1234567890})
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
vid_path = tmp_path / "demo.mp4"
|
||||
vid_path.write_bytes(b"\x00\x00\x00\x18ftyp" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_video(chat_id="+155****4567", video_path=str(vid_path))
|
||||
|
||||
assert result.success is True
|
||||
assert captured[0]["method"] == "send"
|
||||
assert captured[0]["params"]["attachments"] == [str(vid_path)]
|
||||
assert captured[0]["params"]["message"] == "" # caption=None → ""
|
||||
adapter._stop_typing_indicator.assert_awaited_once_with("+155****4567")
|
||||
assert 1234567890 in adapter._recent_sent_timestamps
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_missing_file(self, monkeypatch):
|
||||
"""send_video should fail for nonexistent video."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
result = await adapter.send_video(chat_id="+155****4567", video_path="/missing.mp4")
|
||||
|
||||
assert result.success is False
|
||||
assert "not found" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_too_large(self, monkeypatch, tmp_path):
|
||||
"""send_video should reject files over 100MB."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
vid_path = tmp_path / "huge.mp4"
|
||||
vid_path.write_bytes(b"x")
|
||||
|
||||
def mock_stat(self, **kwargs):
|
||||
class FakeStat:
|
||||
st_size = 200 * 1024 * 1024
|
||||
return FakeStat()
|
||||
|
||||
with patch.object(Path, "stat", mock_stat):
|
||||
result = await adapter.send_video(chat_id="+155****4567", video_path=str(vid_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "too large" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_video_rpc_failure(self, monkeypatch, tmp_path):
|
||||
"""send_video should return error when RPC returns None."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
mock_rpc, _ = _stub_rpc(None)
|
||||
adapter._rpc = mock_rpc
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
vid_path = tmp_path / "demo.mp4"
|
||||
vid_path.write_bytes(b"\x00\x00\x00\x18ftyp" + b"\x00" * 100)
|
||||
|
||||
result = await adapter.send_video(chat_id="+155****4567", video_path=str(vid_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "failed" in result.error.lower()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# MEDIA: tag extraction integration
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSignalMediaExtraction:
|
||||
"""Verify the full pipeline: MEDIA: tag → extract → send_image_file/send_voice."""
|
||||
|
||||
def test_extract_media_finds_image_tag(self):
|
||||
"""BasePlatformAdapter.extract_media should find MEDIA: image paths."""
|
||||
from gateway.platforms.base import BasePlatformAdapter
|
||||
media, cleaned = BasePlatformAdapter.extract_media(
|
||||
"Here's the chart.\nMEDIA:/tmp/price_graph.png"
|
||||
)
|
||||
assert len(media) == 1
|
||||
assert media[0][0] == "/tmp/price_graph.png"
|
||||
assert "MEDIA:" not in cleaned
|
||||
|
||||
def test_extract_media_finds_audio_tag(self):
|
||||
"""BasePlatformAdapter.extract_media should find MEDIA: audio paths."""
|
||||
from gateway.platforms.base import BasePlatformAdapter
|
||||
media, cleaned = BasePlatformAdapter.extract_media(
|
||||
"[[audio_as_voice]]\nMEDIA:/tmp/reply.ogg"
|
||||
)
|
||||
assert len(media) == 1
|
||||
assert media[0][0] == "/tmp/reply.ogg"
|
||||
assert media[0][1] is True # is_voice flag
|
||||
|
||||
def test_signal_has_all_media_methods(self, monkeypatch):
|
||||
"""SignalAdapter must override all media send methods used by gateway."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
from gateway.platforms.base import BasePlatformAdapter
|
||||
|
||||
# These methods must NOT be the base class defaults (which just send text)
|
||||
assert type(adapter).send_image_file is not BasePlatformAdapter.send_image_file
|
||||
assert type(adapter).send_voice is not BasePlatformAdapter.send_voice
|
||||
assert type(adapter).send_video is not BasePlatformAdapter.send_video
|
||||
assert type(adapter).send_document is not BasePlatformAdapter.send_document
|
||||
assert type(adapter).send_image is not BasePlatformAdapter.send_image
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# send_document now routes through _send_attachment (#5105 bonus)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
class TestSignalSendDocumentViaHelper:
|
||||
"""Verify send_document gained size check and path-in-error via _send_attachment."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_document_too_large(self, monkeypatch, tmp_path):
|
||||
"""send_document should now reject files over 100MB (was previously missing)."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
doc_path = tmp_path / "huge.pdf"
|
||||
doc_path.write_bytes(b"x")
|
||||
|
||||
def mock_stat(self, **kwargs):
|
||||
class FakeStat:
|
||||
st_size = 200 * 1024 * 1024
|
||||
return FakeStat()
|
||||
|
||||
with patch.object(Path, "stat", mock_stat):
|
||||
result = await adapter.send_document(chat_id="+155****4567", file_path=str(doc_path))
|
||||
|
||||
assert result.success is False
|
||||
assert "too large" in result.error.lower()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_document_error_includes_path(self, monkeypatch):
|
||||
"""send_document error message should include the file path."""
|
||||
adapter = _make_signal_adapter(monkeypatch)
|
||||
adapter._stop_typing_indicator = AsyncMock()
|
||||
|
||||
result = await adapter.send_document(chat_id="+155****4567", file_path="/nonexistent.pdf")
|
||||
|
||||
assert result.success is False
|
||||
assert "/nonexistent.pdf" in result.error
|
||||
|
||||
Reference in New Issue
Block a user