Skip to content

feat(email): Markdown-to-HTML rendering for all 4 send paths - #73294

Open
swissly wants to merge 4 commits into
NousResearch:mainfrom
swissly:feat/email-html-markdown-rendering
Open

feat(email): Markdown-to-HTML rendering for all 4 send paths#73294
swissly wants to merge 4 commits into
NousResearch:mainfrom
swissly:feat/email-html-markdown-rendering

Conversation

@swissly

@swissly swissly commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds automatic Markdown-to-HTML rendering for all outbound emails. Sends emails as multipart/alternative with both text/plain (raw body) and text/html (styled HTML with inline CSS), so HTML-capable clients render rich formatting while plain-text clients fall back gracefully.

Addresses: Closes #11941

How it works

  1. _markdown_to_html_email(body) converts Markdown to HTML using the markdown library (tables, fenced code, nl2br), then injects inline CSS per element (because Gmail strips <style> blocks).

  2. All 4 send paths are updated:

    • _send_emailMIMEMultipart("alternative") + _attach_body(msg, body)
    • _send_email_with_attachment_create_body_part(body) inside multipart/mixed
    • _send_email_with_attachments_create_body_part(body) inside multipart/mixed
    • _standalone_sendMIMEMultipart("alternative") with html_format config check
  3. Config opt-out: platforms.email.html_format: false disables HTML (default: true).

  4. Graceful fallback: if markdown is not installed or conversion fails, plain text is sent as before.

  5. Security: allowlist sanitizer (_EmailHtmlSanitizer) strips event handlers, javascript:/data: URLs, and <script>/<style> blocks on all outbound HTML.

  6. Already-HTML bodies: _is_html() detects them, _trim_html_preamble_postamble() strips cron-wrapper/model commentary before the first HTML tag and trailing prose after </html> or the last block-level tag (ported from fix(email): HTML emails sent as plain text — two critical bugs #36853, commit 592c9b2f), then the sanitizer applies.

  7. Standalone SMTP fix: _standalone_send uses SMTP_SSL on port 465 (implicit TLS) instead of SMTP+STARTTLS, matching _connect_smtp (found by live E2E against smtp.breitband.ch:465).

Supersedes / Overlapping PRs

Rebase note (2026-08-03)

Branch was 3714 files behind main. Rebuilt fresh from main (commit 9529ee30), carrying the full review history (Copilot + Tecnium rounds on the prior base). Rebased again onto current main (2026-08-03, +149 commits) — only the 4 email commits are in this branch. Test suite: tests/gateway/test_email_html.py (25 tests) + tests/gateway/test_email.py all pass (61 total).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Markdown-to-HTML rendering for outbound emails in the Email platform plugin, aiming to send multipart/alternative messages (plain + HTML) across all outbound send paths, with a platforms.email.html_format opt-out.

Changes:

  • Introduces a Markdown → styled-HTML renderer using inline CSS suitable for email clients.
  • Adds shared helpers to attach plain + optional HTML body parts, and wires them into reply + attachment send paths.
  • Extends the standalone SMTP sender path to optionally send HTML alongside plain text.
Comments suppressed due to low confidence (1)

plugins/platforms/email/adapter.py:1023

  • With html_format: false, _send_email() still constructs a multipart/alternative message (and attachment paths still nest an alternative part). This diverges from the PR/issue expectation that opting out preserves the prior plain-text-only MIME shape. Consider using a plain MIMEText message when HTML is disabled, and only creating multipart/alternative when html_format is enabled.
        """Send an email via SMTP. Runs in executor thread."""
        msg = MIMEMultipart("alternative")
        msg["From"] = self._address
        msg["To"] = to_addr


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

Comment thread plugins/platforms/email/adapter.py Outdated
Comment on lines +96 to +113
# Regex to match <code> inside <pre> blocks — override inline code styling
_PRE_CODE_OVERRIDE = (
re.compile(r"(<pre[^>]*>.*?)<code(\s|>)", re.DOTALL),
r'\1<code style="background:transparent;padding:0;color:inherit;font-size:13px;font-family:Menlo,Monaco,Consolas,monospace;"\2',
)


def _markdown_to_html_email(body: str) -> str:
"""Convert Markdown body to styled HTML email content."""
import markdown as _md_mod
html = _md_mod.markdown(body, extensions=["tables", "fenced_code", "nl2br"])
# Inject inline styles per element (Gmail strips <style> blocks)
for tag, style in _HERMES_EMAIL_STYLES:
html = re.sub(rf"<{tag}(\s|>)", rf"<{tag} {style}\1", html)
# Override <code> inside <pre> blocks (two-pass: inline code already styled above)
html = _PRE_CODE_OVERRIDE[0].sub(_PRE_CODE_OVERRIDE[1], html)
# Use .replace() instead of .format() — body may contain { } braces
return _HERMES_EMAIL_HTML_TEMPLATE.replace("{body}", html)
Comment on lines +1005 to +1011
container.attach(MIMEText(body, "plain", "utf-8"))
if self._html_format:
try:
html = _markdown_to_html_email(body)
container.attach(MIMEText(html, "html", "utf-8"))
except Exception as e:
logger.warning("[Email] HTML conversion failed, sending plain only: %s", e, exc_info=True)
Comment on lines +518 to +520
# HTML email formatting — config: platforms.email.html_format (default: true)
self._html_format = extra.get("html_format", True)

Comment thread plugins/platforms/email/adapter.py Outdated
Comment on lines +997 to +1001
def _create_body_part(self, body: str) -> MIMEMultipart:
"""Create a multipart/alternative body part (for use inside multipart/mixed)."""
alt = MIMEMultipart("alternative")
self._attach_parts(alt, body)
return alt
@alt-glitch alt-glitch added type/feature New feature or request comp/plugins Plugin system and bundled plugins platform/email Email (IMAP/SMTP) adapter P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #54107 is the earlier open live-path HTML-email implementation. Its current diff preserves detected existing HTML and has regression coverage, whereas this patch always Markdown-renders the body. These are competing rendering contracts, so a maintainer should choose/consolidate rather than treat them as duplicates.

@swissly

swissly commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Copilot Review — All 4 findings addressed ✅

1. Duplicate style= in <pre><code> blocks
Replaced two-pass approach with placeholder strategy: extract <pre><code>...\code></pre> blocks into placeholders BEFORE style injection, then restore with properly styled blocks. No duplicate style= attributes possible.

2. ImportError handling
Caught ImportError separately in _attach_parts(). When markdown is not installed (expected opt-in), logs at DEBUG level instead of WARNING. WARNING reserved for real conversion errors.

3. Unit tests ✅ (10 tests, all passing)

  • test_fenced_code_no_duplicate_style — verifies no duplicate style= in pre/code tags
  • test_html_enabled/disabled — verifies MIME structure (2 parts vs 1)
  • test_importerror_falls_back — verifies graceful degradation without markdown
  • test_create_body_part — verifies return type per html_format

4. _create_body_part plain-only
Now returns simple MIMEText when html_format=False, instead of always building multipart/alternative. Attachment emails stay plain-only shape under opt-out.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for porting the Markdown-to-HTML work to the live email adapter. The feature premise is valid: current main sends only MIMEText(..., "plain") in the primary path (plugins/platforms/email/adapter.py:949), both attachment paths (:1063, :1143), and standalone delivery (:1219).

Problems

  • plugins/platforms/email/adapter.py:109-126 converts and styles the entire body without a raw-HTML safety step. The existing Matrix rich-output path pre-sanitizes before conversion (plugins/platforms/matrix/adapter.py:4561, :837-854) and allowlist-sanitizes output (:816-823). The original issue discussion also raised disabling raw HTML/JS. Please define and test the email policy before shipping this renderer.
  • The member discussion correctly identifies a competing contract with #54107: it preserves detected existing HTML, while this patch always Markdown-renders. This needs consolidation rather than an implicit behavior change.
  • website/docs/user-guide/messaging/email.md:123 still says replies are plain text, but this PR adds a public html_format option without documentation.

Suggested changes

  • Add an explicit sanitized/raw-HTML policy with security regression tests, reconcile it with #54107, and document html_format plus MIME behavior.
  • Replace the tautological brace assertion in tests/gateway/test_email_html.py:48 with an exact rendering assertion.

Automated hermes-sweeper review.

def _markdown_to_html_email(body: str) -> str:
"""Convert Markdown body to styled HTML email content."""
import markdown as _md_mod
html = _md_mod.markdown(body, extensions=["tables", "fenced_code", "nl2br"])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renderer converts and styles the full body without a raw-HTML safety step. The established Matrix path pre-sanitizes Markdown and allowlist-sanitizes rendered HTML (plugins/platforms/matrix/adapter.py:837-854, :816-823). Please define and test the email policy for raw HTML, event attributes, and unsafe URLs before sending it as text/html.

Comment thread tests/gateway/test_email_html.py Outdated
"""Body with { } braces must not break template substitution."""
from plugins.platforms.email.adapter import _markdown_to_html_email
html = _markdown_to_html_email("Use `{code}` here")
assert "{code}" not in html or "code" in html # braces consumed or preserved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is tautological: the generated template/styles already contain the substring code, so the right-hand side makes it pass regardless of brace handling. Assert the exact expected rendered code span or verify conversion completes while preserving the intended content.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@swissly

swissly commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Tecnium Review — All findings addressed ✅

1. HTML sanitization policy
Added _EmailHtmlSanitizer — allowlist-based HTML sanitizer mirroring the Matrix adapter pattern:

  • Only safe structural tags (p, h1-h6, ul, ol, table, a, code, pre, strong, em, etc.)
  • Event handlers (onclick, onload, etc.) stripped
  • Unsafe URLs (javascript:, data:) stripped
  • <script>/<style> blocks removed entirely
  • Inline styles preserved (email clients need them)
  • Sanitization applied AFTER Markdown-to-HTML conversion
  • 8 security regression tests (script, events, javascript URLs, safe URLs, style tags, inline styles, disallowed/allowed tags)

2. HTML detection for pre-existing HTML
Added _is_html() — detects block-level HTML tags in body. When body already contains HTML (e.g. from a skill that generates HTML), sanitizes directly instead of re-rendering through Markdown. Resolves conflict with #54107 behavior.

3. Documentation updated
website/docs/user-guide/messaging/email.md now documents:

  • multipart/alternative MIME structure
  • html_format config option (default: true)
  • MIME structure with/without attachments
  • Sanitization policy

4. Test fixes

  • test_braces_not_escaped: replaced tautological assertion with exact rendering check
  • 10 new tests: 8 security + 2 HTML detection
  • Total: 20 tests, all passing (1.16s)

@swissly

swissly commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Test comment via gh CLI

@swissly

swissly commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (2026-08-03, +149 commits upstream). Branch was rebuilt clean from origin/main — only the 3 email commits (HTML rendering, 465 SMTP_SSL, tests) are included; prior local patches (tool-call repair, OpenViking) are no longer in this branch. 54/54 email tests green. PR #34603 (overlap) was closed by its author — this PR remains the consolidated HTML-email fix. Note: upstream merged secret-scope fixes (f08f403, 359ff01) which our standalone-send path now composes with cleanly.

swissly added a commit to swissly/hermes-agent that referenced this pull request Aug 3, 2026
Nested escaped text like &amp;lt;APIKEY&amp;gt; was decoded twice in one
_strip_html call (&amp; -> &, then &lt; -> <), producing <APIKEY> instead
of literal &lt;APIKEY&gt;. Decode &lt;/&gt; before &amp; so each entity
layer is decoded at most once (issue NousResearch#68704).

Same fix as PR NousResearch#68707 (tooiuiiu); applied locally for immediate effect,
kept out of PR NousResearch#73294 (send-path scope). Tests: +2 regression cases.
@swissly

swissly commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Triage update for the needs-decision label: the earlier live-path implementation referenced during triage — #54107 — has been closed without merging, so there is no longer a competing contract to reconcile. This PR (Markdown→HTML for all 4 send paths, allowlist sanitizer, 54/54 tests, rebased onto current main 2026-08-03) is MERGEABLE/CLEAN and ready for review. Happy to adjust scope if a narrower change is preferred.

Resolves the unresolved merge conflict left by hermes update (main had
UU state on adapter.py): HEAD kept secret-scope reads, incoming kept the
HTML block. Both merged; sanitizer + placeholder strategy ported from
72c960d91a (both Copilot and Tecnium reviews passed).

- _markdown_to_html_email: placeholder strategy for <pre><code> blocks
  (no duplicate style=), allowlist sanitizer (_EmailHtmlSanitizer),
  _is_html detection for already-HTML bodies
- _attach_parts: plain + HTML in multipart/alternative, ImportError
  separated from general Exception
- _create_body_part: returns MIMEText when html_format disabled
- _standalone_send: honors html_format (was hardcoded plain), uses
  _get_secret scope-aware reads (conflict merge, not os.getenv)
- tests: 20 new tests (markdown, MIME structure, sanitization, detection)

Supersedes prior branch base (3714 files stale); fresh branch off main.
_standalone_send (cron delivery path) used SMTP()+STARTTLS for ALL ports;
port 465 requires implicit TLS. Matches _connect_smtp behavior (class path).
Found by live E2E test against smtp.breitband.ch:465 (timeout without fix).
Regression tests for the port-selection fix found by live E2E test
(smtp.breitband.ch:465 hung with plain SMTP+STARTTLS).
Ported from PR NousResearch#36853 (chtse53): cron wrappers ('Cronjob Response: <name>')
and model commentary before the first HTML tag, plus trailing prose after
</html> or the last block-level tag, are stripped before sanitizing an
already-HTML body. Prevents broken plain-text wrappers from rendering
inside HTML emails.

NousResearch#36853 targets gateway/platforms/email.py which no longer exists on main
(adapters migrated to bundled plugins in 5600105); this ports the
feature to plugins/platforms/email/adapter.py. Contributor credit kept.
5 regression tests.
@swissly
swissly force-pushed the feat/email-html-markdown-rendering branch from 919258f to e1a695b Compare August 12, 2026 14:50
@swissly

swissly commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Ping @teknium1 — this PR is mergeable/clean, all Copilot+Tecnium findings addressed, 54 email tests green, rebased onto current main. Carries the needs-decision label (the competing #54107 is closed). Requesting a maintainer decision / review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have platform/email Email (IMAP/SMTP) adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: HTML email support for email gateway (multipart/alternative + Markdown rendering)

4 participants