feat(signal): add media send methods and disk-based attachment fallback by gibbsoft · Pull Request #3704 · NousResearch/hermes-agent

gibbsoft · 2026-03-29T10:31:09Z

Summary

Add send_image_file, send_voice, send_video overrides to Signal adapter, routing through send_document for native media attachments
Add disk-based attachment reading from ~/.local/share/signal-cli/attachments/ as primary method, falling back to getAttachment RPC
Add debug/warning logging for attachment processing

Motivation

Signal adapter currently falls back to sending file paths as text (e.g. 🖼️ Image: /path/to/file) because the base class media methods aren't overridden. The getAttachment RPC can also fail when the account identifier doesn't match, while the files are always available on disk.

Test plan

Send an image to hermes via Signal — verify it's received and processed (disk-based read)
Have hermes generate/send an image back via Signal — verify it arrives as an attachment, not a file path
Test audio and video send paths

Add send_image_file, send_voice, send_video overrides that route through send_document for native Signal media attachments. Add disk-based attachment reading from signal-cli attachments directory as primary method, falling back to getAttachment RPC. Improves reliability when RPC returns errors.

Copilot

Pull request overview

This PR enhances the Signal platform adapter to support native media attachments for outbound messages and improves inbound attachment retrieval by preferring on-disk signal-cli attachment files with an RPC fallback.

Changes:

Add send_image_file, send_voice, and send_video overrides that route through send_document for attachment-based delivery.
Update _fetch_attachment to attempt reading from ~/.local/share/signal-cli/attachments/ before falling back to getAttachment JSON-RPC.
Add debug/warning logging around attachment processing and failure modes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-29T10:34:02Z

gateway/platforms/signal.py

+        att_dir = os.path.expanduser("~/.local/share/signal-cli/attachments/")
+        matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))
+        if matches:
+            disk_path = matches[0]
+            try:
+                with open(disk_path, "rb") as f:
+                    raw_data = f.read()


Disk-based attachment loading reads the entire file without validating its on-disk size. This bypasses the earlier att_size check (which might be missing/incorrect) and allows unexpectedly large reads; check the file size with stat() and enforce SIGNAL_MAX_ATTACHMENT_SIZE before reading.

Copilot · 2026-03-29T10:34:02Z

gateway/platforms/signal.py

-            result = result.get("data")
            if not result:
-                logger.warning("Signal: attachment response missing 'data' key")
+                logger.warning("Signal: failed to fetch attachment %s - got null path", attachment_id)


The warning message "got null path" is misleading here: getAttachment returns base64 data (or a dict with a data field), not a file path. Consider rewording to reflect an empty/None RPC result so logs are actionable.

Suggested change

logger.warning("Signal: failed to fetch attachment %s - got null path", attachment_id)

logger.warning("Signal: failed to fetch attachment %s - got empty RPC result (no attachment data)", attachment_id)

Copilot · 2026-03-29T10:34:03Z

gateway/platforms/signal.py

+        matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))
+        if matches:
+            disk_path = matches[0]


Using glob with an unvalidated attachment_id and the pattern f"{attachment_id}*" can match multiple files and the code picks matches[0] (non-deterministic ordering). Consider validating attachment_id (e.g., disallow path separators/glob metacharacters) and selecting the best match deterministically (exact match first, else newest by mtime).

Suggested change

matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))

if matches:

disk_path = matches[0]

# Validate attachment_id before using it in a glob pattern to avoid

# unintended glob metacharacters or path traversal.

if re.fullmatch(r"[A-Za-z0-9._-]+", attachment_id):

pattern = os.path.join(att_dir, f"{attachment_id}*")

matches = glob.glob(pattern)

else:

matches = []

selected_path = None

if matches:

# Prefer an exact filename match (basename without extension equals attachment_id)

exact_matches = []

for m in matches:

base = os.path.basename(m)

name_without_ext, _ = os.path.splitext(base)

if name_without_ext == attachment_id:

exact_matches.append(m)

if exact_matches:

# Choose deterministically among exact matches

selected_path = sorted(exact_matches)[0]

else:

# Fallback: choose the newest matching file by modification time

try:

selected_path = max(matches, key=os.path.getmtime)

except (OSError, ValueError):

selected_path = None

if selected_path:

disk_path = selected_path

Copilot · 2026-03-29T10:34:03Z

gateway/platforms/signal.py

+    async def send_image_file(
+        self,
+        chat_id: str,
+        image_path: str,
+        caption: Optional[str] = None,
+        **kwargs,
+    ) -> SendResult:
+        """Send a local image file as an attachment."""
+        return await self.send_document(chat_id, image_path, caption)
+
+    async def send_voice(
+        self,
+        chat_id: str,
+        audio_path: str,
+        caption: Optional[str] = None,
+        **kwargs,
+    ) -> SendResult:
+        """Send an audio file as an attachment."""
+        return await self.send_document(chat_id, audio_path, caption)
+
+    async def send_video(
+        self,
+        chat_id: str,
+        video_path: str,
+        caption: Optional[str] = None,
+        **kwargs,
+    ) -> SendResult:
+        """Send a video file as an attachment."""
+        return await self.send_document(chat_id, video_path, caption)


send_image_file/send_voice/send_video accept **kwargs but don't forward them to send_document. This drops any caller-provided options (e.g., reply_to, metadata, file_name) and is inconsistent with other adapters; pass **kwargs through to send_document to preserve the base adapter contract.

Copilot · 2026-03-29T10:34:03Z

gateway/platforms/signal.py

    async def _fetch_attachment(self, attachment_id: str) -> tuple:
-        """Fetch an attachment via JSON-RPC and cache it. Returns (path, ext)."""
-        result = await self._rpc("getAttachment", {
-            "account": self.account,
-            "id": attachment_id,
-        })
+        """Fetch an attachment via JSON-RPC and cache it. Returns (path, ext).
+
+        Tries reading from the signal-cli attachments directory first,
+        falling back to the getAttachment RPC call.
+        """
+        raw_data = None
+
+        # Try disk-based read first (signal-cli stores attachments locally)
+        att_dir = os.path.expanduser("~/.local/share/signal-cli/attachments/")
+        matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))
+        if matches:
+            disk_path = matches[0]
+            try:
+                with open(disk_path, "rb") as f:
+                    raw_data = f.read()
+                logger.debug("Signal: read attachment %s from disk (%d bytes)", attachment_id, len(raw_data))
+            except Exception as e:
+                logger.warning("Signal: failed to read attachment from disk %s: %s", disk_path, e)
+                raw_data = None

-        if not result:
-            return None, ""
+        # Fall back to JSON-RPC if disk read didn't work
+        if raw_data is None:
+            result = await self._rpc("getAttachment", {
+                "account": self.account,
+                "id": attachment_id,
+            })


The new disk-first attachment branch in _fetch_attachment() isn't covered by tests. Since tests/gateway/test_signal.py already covers the RPC path, add a test that stubs glob/open to return bytes and asserts rpc("getAttachment") is not called (and that the correct cache* helper is used).

Copilot · 2026-03-29T10:34:03Z

gateway/platforms/signal.py

+        att_dir = os.path.expanduser("~/.local/share/signal-cli/attachments/")
+        matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))
+        if matches:
+            disk_path = matches[0]
+            try:
+                with open(disk_path, "rb") as f:
+                    raw_data = f.read()
+                logger.debug("Signal: read attachment %s from disk (%d bytes)", attachment_id, len(raw_data))


_fetch_attachment() does synchronous disk I/O (open/read) inside an async method. For large attachments (up to 100MB) this can block the event loop; offload the file read to a thread (e.g., via asyncio.to_thread) or use an async file reader so inbound SSE processing stays responsive.

Copilot AI review requested due to automatic review settings March 29, 2026 10:31

Copilot started reviewing on behalf of gibbsoft March 29, 2026 10:31 View session

gibbsoft mentioned this pull request Mar 29, 2026

fix(signal): URL-encode account in SSE query string and add read receipts #1958

Closed

Copilot AI reviewed Mar 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(signal): add media send methods and disk-based attachment fallback#3704

feat(signal): add media send methods and disk-based attachment fallback#3704
gibbsoft wants to merge 1 commit intoNousResearch:mainfrom
gibbsoft:feat/signal-media-send

gibbsoft commented Mar 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Copilot AI Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	logger.warning("Signal: failed to fetch attachment %s - got null path", attachment_id)
	logger.warning("Signal: failed to fetch attachment %s - got empty RPC result (no attachment data)", attachment_id)

-        matches = glob.glob(os.path.join(att_dir, f"{attachment_id}*"))
-        if matches:
-            disk_path = matches[0]
+        # Validate attachment_id before using it in a glob pattern to avoid
+        # unintended glob metacharacters or path traversal.
+        if re.fullmatch(r"[A-Za-z0-9._-]+", attachment_id):
+            pattern = os.path.join(att_dir, f"{attachment_id}*")
+            matches = glob.glob(pattern)
+        else:
+            matches = []
+        selected_path = None
+        if matches:
+            # Prefer an exact filename match (basename without extension equals attachment_id)
+            exact_matches = []
+            for m in matches:
+                base = os.path.basename(m)
+                name_without_ext, _ = os.path.splitext(base)
+                if name_without_ext == attachment_id:
+                    exact_matches.append(m)
+            if exact_matches:
+                # Choose deterministically among exact matches
+                selected_path = sorted(exact_matches)[0]
+            else:
+                # Fallback: choose the newest matching file by modification time
+                try:
+                    selected_path = max(matches, key=os.path.getmtime)
+                except (OSError, ValueError):
+                    selected_path = None
+        if selected_path:
+            disk_path = selected_path

Conversation

gibbsoft commented Mar 29, 2026

Summary

Motivation

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants