fix(gateway): add Signal message type classification for documents#12851
fix(gateway): add Signal message type classification for documents#12851kdunn926 wants to merge 3 commits into
Conversation
ether-btc
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review — PR #12851
Verdict: Request Changes
Clean fix with good test coverage. One gap: no tests for the document message-type classification logic.
🔴 Critical — No Tests for Document Message-Type Classification
File: gateway/platforms/signal.py:509
The fix adds application/* and text/* MIME type handling to classify attachments as MessageType.DOCUMENT. However, the test suite does not include a test case for this specific classification.
The existing tests in TestSignalInboundMessageTypeClassification (test_pdf_attachment_sets_document_type) test application/pdf, which is covered. But text/* is a broader type — text/plain, text/csv, text/html — and no test verifies these are classified as DOCUMENT.
This matters because MessageType.DOCUMENT is the gate for run.py's document-context injection. If a future regression narrows the MIME type check, or if text/* behaves differently than application/*, there is no test to catch it.
Suggested fix:
- Add a test with
contentType: "text/plain"(e.g., a.txtattachment) and verify it is classified asDOCUMENT - Add a test with
contentType: "text/html"to cover the fulltext/*wildcard
✅ Looks Good
- Correctly extends the existing MIME type chain with
elif— maintains the priority order (voice → photo → document) - Good test structure using
_make_dm_envelopehelper withAsyncMockfor_fetch_attachment - Clear docstring explaining the bug: documents misclassified as TEXT caused silent file drops
Reviewed by Hermes Agent
Hermes Agent Code Review — PR #12851Verdict: APPROVE Clean, minimal fix with good test coverage. Solves a real user-facing bug (PDFs silently dropped) with surgical precision. 🟢 Nit — Add Type Hint to _dispatch_single_attachmentFile: The helper method # Current:
async def _dispatch_single_attachment(self, monkeypatch, content_type: str,
att_id: str, fetch_path: str, fetch_ext: str):
...
# Suggested:
async def _dispatch_single_attachment(self, monkeypatch, content_type: str,
att_id: str, fetch_path: str, fetch_ext: str) -> InboundEvent:
...Impact: Low - IDE/type-checker doesn't know the return type. Adding 🟢 Nit — Import InboundEvent at Module LevelFile: Each test imports # Add at top of file with other imports:
from gateway.platforms.base import MessageType, InboundEventThen remove the inline imports from each test method. Impact: Low - current code works fine. This is purely style. What Works Well
Code Quality Assessment
Security Assessment✅ No security concerns. This change only affects message type classification. The Performance Assessment✅ No performance concerns. The Bug Detection✅ Logic is sound. The elif chain correctly handles the priority order:
Tests verify the new DOCUMENT classification for key MIME types. RecommendationAPPROVE — This is a solid fix. The two nits above are minor style improvements that could be addressed in a follow-up, but they don't block merge. The PR resolves issue #12845 cleanly: users can now send PDFs and other document attachments via Signal and the agent will receive them properly. |
ether-btc
left a comment
There was a problem hiding this comment.
LGTM with nits noted. Clean fix, good test coverage.
ether-btc
left a comment
There was a problem hiding this comment.
Hermes Agent Code Review — PR #12851
Verdict: APPROVE
Clean, minimal fix with good test coverage. Solves a real user-facing bug (PDFs silently dropped) with surgical precision.
🟢 Nit — Add Type Hint to _dispatch_single_attachment
File: tests/gateway/test_signal.py:672
The helper method _dispatch_single_attachment returns dispatched[0] which is an InboundEvent, but lacks a return type annotation.
Impact: Low - IDE/type-checker doesn't know the return type. Adding -> InboundEvent improves static analysis.
🟢 Nit — Import InboundEvent at Module Level
File: tests/gateway/test_signal.py
Each test imports from gateway.platforms.base import MessageType inline (lines 709, 731, 753). This works but is slightly inconsistent with Python style.
Impact: Low - current code works fine. This is purely style.
What Works Well
- Surgical fix — Only 2 lines changed in production code, exactly where needed
- Good test coverage — 3 tests cover PDF, text/plain, text/html (the key MIME types)
- Clear intent — Comment in test docstring explains the bug and fix
- Proper ordering — elif chain correctly prioritizes audio → image → document → text
- Edge case handling — Tests verify that wildcards work for various
text/*subtypes
Code Quality Assessment
| Area | Score | Notes |
|---|---|---|
| Code organization | ✅ | Helper method keeps tests DRY |
| Naming | ✅ | Clear, descriptive names |
| Test coverage | ✅ | Covers key MIME types |
| Error handling | N/A | N/A for this change |
Security Assessment
✅ No security concerns. This change only affects message type classification. The media_types list comes from Signal-CLI's validated envelope data. No new attack surface.
Performance Assessment
✅ No performance concerns. The any() calls are O(n) on attachment count (typically 1-5). String prefix checks are negligible.
Bug Detection
✅ Logic is sound. The elif chain correctly handles the priority order:
audio/→ VOICEimage/→ PHOTOapplication/ortext/→ DOCUMENT (new)- fallback → TEXT
Tests verify that the new DOCUMENT classification works for key MIME types.
Recommendation
APPROVE — This is a solid fix. The two nits above are minor style improvements that could be addressed in a follow-up, but they don't block merge.
The PR resolves issue #12845 cleanly: users can now send PDFs and other document attachments via Signal and the agent will receive them properly.
1 similar comment
|
Superseded by #20347 which centralizes inbound attachment classification logic and adds broader MIME coverage (DOCUMENT + VIDEO). |
…fallback Widen the salvaged #12851 fix to match the established classification pattern (WhatsApp/Slack/BlueBubbles/Mattermost): video/* -> VIDEO, and any remaining MIME type falls through to DOCUMENT instead of TEXT, so exotic types still trigger run.py's document-context injection.
What does this PR do?
Add missing Signal adapter
DOCUMENTmessage type for non-image/non-audio attachments.Related Issue
Fixes #12845
Type of Change
Changes Made
src/gateway/platforms/signal.pyto include support forapplication/*andtext/*MIME types.How to Test
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