fix: URL-encode Signal phone numbers and correct attachment RPC parameter (#3670)

Fixes two Signal bugs:

1. SSE connection: URL-encode phone numbers so + isn't interpreted as space (400 Bad Request)
2. Attachment fetch: use 'id' parameter instead of 'attachmentId' (NullPointerException in signal-cli)

Also refactors Signal tests with shared helpers.

gateway/platforms/signal.py

@@ -22,7 +22,7 @@ import time
 from datetime import datetime, timezone
 from pathlib import Path
 from typing import Dict, List, Optional, Any
-from urllib.parse import unquote
+from urllib.parse import quote, unquote
 
 import httpx
 
@@ -253,7 +253,7 @@ class SignalAdapter(BasePlatformAdapter):
 
     async def _sse_listener(self) -> None:
         """Listen for SSE events from signal-cli daemon."""
-        url = f"{self.http_url}/api/v1/events?account={self.account}"
+        url = f"{self.http_url}/api/v1/events?account={quote(self.account, safe='')}"
         backoff = SSE_RETRY_DELAY_INITIAL
 
         while self._running:
@@ -521,7 +521,7 @@ class SignalAdapter(BasePlatformAdapter):
         """Fetch an attachment via JSON-RPC and cache it. Returns (path, ext)."""
         result = await self._rpc("getAttachment", {
             "account": self.account,
-            "attachmentId": attachment_id,
+            "id": attachment_id,
         })
 
         if not result:

tests/gateway/test_signal.py

@@ -1,11 +1,42 @@
 """Tests for Signal messenger platform adapter."""
+import base64
 import json
 import pytest
 from unittest.mock import MagicMock, patch, AsyncMock
+from urllib.parse import quote
 
 from gateway.config import Platform, PlatformConfig
 
 
+# ---------------------------------------------------------------------------
+# Shared Helpers
+# ---------------------------------------------------------------------------
+
+def _make_signal_adapter(monkeypatch, account="+15551234567", **extra):
+    """Create a SignalAdapter with sensible test defaults."""
+    monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", extra.pop("group_allowed", ""))
+    from gateway.platforms.signal import SignalAdapter
+    config = PlatformConfig()
+    config.enabled = True
+    config.extra = {
+        "http_url": "http://localhost:8080",
+        "account": account,
+        **extra,
+    }
+    return SignalAdapter(config)
+
+
+def _stub_rpc(return_value):
+    """Return an async mock for SignalAdapter._rpc that captures call params."""
+    captured = []
+
+    async def mock_rpc(method, params, rpc_id=None):
+        captured.append({"method": method, "params": dict(params)})
+        return return_value
+
+    return mock_rpc, captured
+
+
 # ---------------------------------------------------------------------------
 # Platform & Config
 # ---------------------------------------------------------------------------
@@ -61,48 +92,22 @@ class TestSignalConfigLoading:
 # ---------------------------------------------------------------------------
 
 class TestSignalAdapterInit:
-    def _make_config(self, **extra):
-        config = PlatformConfig()
-        config.enabled = True
-        config.extra = {
-            "http_url": "http://localhost:8080",
-            "account": "+15551234567",
-            **extra,
-        }
-        return config
-
     def test_init_parses_config(self, monkeypatch):
-        monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "group123,group456")
-
-        from gateway.platforms.signal import SignalAdapter
-        adapter = SignalAdapter(self._make_config())
-
+        adapter = _make_signal_adapter(monkeypatch, group_allowed="group123,group456")
         assert adapter.http_url == "http://localhost:8080"
         assert adapter.account == "+15551234567"
         assert "group123" in adapter.group_allow_from
 
     def test_init_empty_allowlist(self, monkeypatch):
-        monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "")
-
-        from gateway.platforms.signal import SignalAdapter
-        adapter = SignalAdapter(self._make_config())
-
+        adapter = _make_signal_adapter(monkeypatch)
         assert len(adapter.group_allow_from) == 0
 
     def test_init_strips_trailing_slash(self, monkeypatch):
-        monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "")
-
-        from gateway.platforms.signal import SignalAdapter
-        adapter = SignalAdapter(self._make_config(http_url="http://localhost:8080/"))
-
+        adapter = _make_signal_adapter(monkeypatch, http_url="http://localhost:8080/")
         assert adapter.http_url == "http://localhost:8080"
 
     def test_self_message_filtering(self, monkeypatch):
-        monkeypatch.setenv("SIGNAL_GROUP_ALLOWED_USERS", "")
-
-        from gateway.platforms.signal import SignalAdapter
-        adapter = SignalAdapter(self._make_config())
-
+        adapter = _make_signal_adapter(monkeypatch)
         assert adapter._account_normalized == "+15551234567"
 
 
@@ -189,6 +194,73 @@ class TestSignalHelpers:
         assert check_signal_requirements() is False
 
 
+# ---------------------------------------------------------------------------
+# SSE URL Encoding (Bug Fix: phone numbers with + must be URL-encoded)
+# ---------------------------------------------------------------------------
+
+class TestSignalSSEUrlEncoding:
+    """Verify that phone numbers with + are URL-encoded in the SSE endpoint."""
+
+    def test_sse_url_encodes_plus_in_account(self):
+        """The + in E.164 phone numbers must be percent-encoded in the SSE query string."""
+        encoded = quote("+31612345678", safe="")
+        assert encoded == "%2B31612345678"
+
+    def test_sse_url_encoding_preserves_digits(self):
+        """Digits and country codes should pass through URL encoding unchanged."""
+        assert quote("+15551234567", safe="") == "%2B15551234567"
+
+
+# ---------------------------------------------------------------------------
+# Attachment Fetch (Bug Fix: parameter must be "id" not "attachmentId")
+# ---------------------------------------------------------------------------
+
+class TestSignalAttachmentFetch:
+    """Verify that _fetch_attachment uses the correct RPC parameter name."""
+
+    @pytest.mark.asyncio
+    async def test_fetch_attachment_uses_id_parameter(self, monkeypatch):
+        """RPC getAttachment must use 'id', not 'attachmentId' (signal-cli requirement)."""
+        adapter = _make_signal_adapter(monkeypatch)
+
+        png_data = b"\x89PNG\r\n\x1a\n" + b"\x00" * 100
+        b64_data = base64.b64encode(png_data).decode()
+
+        adapter._rpc, captured = _stub_rpc({"data": b64_data})
+
+        with patch("gateway.platforms.signal.cache_image_from_bytes", return_value="/tmp/test.png"):
+            await adapter._fetch_attachment("attachment-123")
+
+        call = captured[0]
+        assert call["method"] == "getAttachment"
+        assert call["params"]["id"] == "attachment-123"
+        assert "attachmentId" not in call["params"], "Must NOT use 'attachmentId' — causes NullPointerException in signal-cli"
+        assert call["params"]["account"] == "+15551234567"
+
+    @pytest.mark.asyncio
+    async def test_fetch_attachment_returns_none_on_empty(self, monkeypatch):
+        adapter = _make_signal_adapter(monkeypatch)
+        adapter._rpc, _ = _stub_rpc(None)
+        path, ext = await adapter._fetch_attachment("missing-id")
+        assert path is None
+        assert ext == ""
+
+    @pytest.mark.asyncio
+    async def test_fetch_attachment_handles_dict_response(self, monkeypatch):
+        adapter = _make_signal_adapter(monkeypatch)
+
+        pdf_data = b"%PDF-1.4" + b"\x00" * 100
+        b64_data = base64.b64encode(pdf_data).decode()
+
+        adapter._rpc, _ = _stub_rpc({"data": b64_data})
+
+        with patch("gateway.platforms.signal.cache_document_from_bytes", return_value="/tmp/test.pdf"):
+            path, ext = await adapter._fetch_attachment("doc-456")
+
+        assert path == "/tmp/test.pdf"
+        assert ext == ".pdf"
+
+
 # ---------------------------------------------------------------------------
 # Session Source
 # ---------------------------------------------------------------------------