fix(signal): classify inbound document attachments - #20347
Conversation
|
I’m hit by the exact issue this PR fixes. Can we get it across the finish line, @alt-glitch? |
|
I just updated it again to be even more robust for other attachments, and to align more closely with existing patterns. |
|
@alt-glitch, would love to get this pushed through. Big lift for privacy-focused users, thanks! |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving Signal document filenames; that part addresses a remaining gap in current main (gateway/platforms/signal.py:680, :915).
Problems
- Current main already handles the PR's DOCUMENT/VIDEO classification goal (
gateway/platforms/signal.py:715-726; commits8207ae888and1e29ab38c). The proposed helper instead makes a document win over audio in mixed media. That conflicts with current per-attachment processing:gateway/run.py:10396-10402suppresses STT forDOCUMENT, while:10512-10551already surfaces documents independently. tests/gateway/test_signal.py:1267in this PR definesTestSignalInboundAttachmentsa second time, replacing the earlier same-named class at module load and leaving the earlier DOCX envelope test uncollected.
Suggested changes
- Salvage the metadata-aware cache-name change only, retaining main's existing message-type selection.
- Remove the duplicate class and add one envelope-level assertion that attachment metadata reaches
_fetch_attachment.
Automated hermes-sweeper review.
| assert event.reply_to_message_id == "123" | ||
| assert event.reply_to_text is None | ||
|
|
||
|
|
There was a problem hiding this comment.
This redefines the earlier TestSignalInboundAttachments class added in this PR. Python replaces the first class binding during module import, so the earlier DOCX envelope test is not collected. Merge these cases into one uniquely defined class.
What does this PR do?
Fixes Signal inbound attachment handling so PDFs and other non-image/non-audio/non-video files arrive as
MessageType.DOCUMENTinstead ofMessageType.TEXT. That restores the document branch ingateway/run.py, so Hermes can surface the saved file path to the agent instead of silently dropping the attachment context.This also preserves inbound document filenames and extensions when caching Signal attachments. If Signal does not provide a filename, the adapter falls back to Hermes' shared
SUPPORTED_DOCUMENT_TYPESmapping before using generic MIME guessing.video/*attachments still remainMessageType.VIDEO.This supersedes #12851 by centralizing the classification logic, adding regression coverage for mixed media and OOXML attachments, and preserving
video/*asMessageType.VIDEOinstead of leaving it as plain text.Related Issue
Fixes #12845
Type of Change
Changes Made
_detect_inbound_message_type()ingateway/platforms/signal.pyto classify inbound Signal attachments from MIME type.MessageType.DOCUMENTso document context injection runs.video/*attachments toMessageType.VIDEOso broader document handling does not regress video behavior._fetch_attachment()so Signal document caching can preserve the user-visible filename when available.SUPPORTED_DOCUMENT_TYPESmapping when Signal omits the filename.image,video,application/pdf, mixed media).MessageType.DOCUMENTare preserved.How to Test
python -m pytest tests/gateway/test_signal.py -qin a normal repo dev environment.MessageType.DOCUMENTinstead ofMessageType.TEXT..docxsuffix instead of degrading to.zip.gateway/run.pyand injects the saved file path into agent context.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs