From 5e88eb2ba0bd9c6087d7d741e20e7bfebf705ceb Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Mon, 6 Apr 2026 20:41:47 +0530
Subject: [PATCH] fix(signal): implement send_image_file, send_voice, and
 send_video for MEDIA: tag delivery
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 gateway/platforms/signal.py  |  74 +++++++-
 tests/gateway/test_signal.py | 339 +++++++++++++++++++++++++++++++++++
 2 files changed, 406 insertions(+), 7 deletions(-)

diff --git a/gateway/platforms/signal.py b/gateway/platforms/signal.py
index 1629e0863..66d455cca 100644
--- a/gateway/platforms/signal.py
+++ b/gateway/platforms/signal.py
@@ -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
diff --git a/tests/gateway/test_signal.py b/tests/gateway/test_signal.py
index acd6513e5..b2830e1fc 100644
--- a/tests/gateway/test_signal.py
+++ b/tests/gateway/test_signal.py
@@ -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