Skip to content

fix(email): send a text/html alternative so Markdown renders - #138554

Open
daniel5simmons wants to merge 1 commit into
NousResearch:mainfrom
daniel5simmons:fix/email-html-alternative
Open

daniel5simmons wants to merge 1 commit into
NousResearch:mainfrom
daniel5simmons:fix/email-html-alternative

Conversation

@daniel5simmons

Copy link
Copy Markdown

What breaks

Email is the only platform whose native rich-text format is HTML rather than Markdown. Every other adapter has a Markdown strategy — Telegram converts to MarkdownV2, Slack to mrkdwn, Matrix to HTML, SMS strips it — so agent output renders there. The email adapter has none, so Markdown the agent writes for chat reaches a mail reader as literal asterisks and hashes.

To reproduce: have the agent write anything with **bold** or a ## heading and deliver it to email. The mail arrives as ## Bottom line / The **PJM** order. A daily cron briefing makes it unmissable; short conversational replies hide it.

Manifests at exactly two lines, on both send paths:

  • plugins/platforms/email/adapter.py:874 — msg.attach(MIMEText(body, "plain", "utf-8")) in _new_reply(), which every in-process send funnels through (replies, send_image, send_multiple_images, send_document)
  • plugins/platforms/email/adapter.py:975 — msg = MIMEText(message, "plain", "utf-8") in _standalone_send(), the out-of-process cron path

The fix

Both paths now attach a multipart/alternative carrying rendered HTML beside the unchanged plain part:

def _body_alternative(body: str) -> MIMEMultipart:
    """Plain part first, HTML second: a client that cannot render HTML keeps the original text."""
    alternative = MIMEMultipart("alternative")
    alternative.attach(MIMEText(body, "plain", "utf-8"))
    alternative.attach(MIMEText(markdown_to_email_html(body), "html", "utf-8"))
    return alternative
  • _new_reply() keeps its MIMEMultipart() wrapper, so attachments stay siblings of the alternative (multipart/mixed → multipart/alternative → (plain, html))
  • _standalone_send() has nothing to attach, so the alternative is the top level

markdown_html.py converts the subset the agent actually emits — headings, bold, italic, inline code, bullets, numbered lists, [links](url) and bare URLs — into inline-styled HTML (mail clients ignore stylesheets). Content is escaped before any Markdown is applied, so a body cannot inject live markup.

The plain part is byte-identical to what was sent before this change. The HTML part is purely additive; a client that cannot render HTML is unaffected.

Testing

tests/gateway/test_email_html_body.py (new) — 3 behaviour contracts:

  1. in-process send: plain part unchanged, HTML alternative renders, untrusted markup escaped
  2. attachment reply: the attachment is a sibling of the alternative, not inside it
  3. _standalone_send(): the sibling call path carries the alternative too

All 3 were proven red on the base by reverting the adapter — they fail, they are not tests written to match the implementation.

Notes for review

  • One existing test was adjusted. test_oversized_cron_output_is_delivered_as_one_whole_email asserted on msg.get_payload()[0], which is now the alternative container rather than the plain part, so it walks to the text/plain part instead. Its contract ("the whole cron payload arrives as one untruncated message") is unchanged, and it passes on the base and with the fix. Flagging it because an author editing a test to go green deserves scrutiny.
  • markdown is not a declared dependency. Matrix solves the same problem with the third-party markdown library, imported there opportunistically under suppress(ImportError) with a regex fallback. Extracting a shared Markdown-to-HTML converter would be the better long-term shape; it is deliberately left as a follow-up rather than scope creep here. This converter adds no dependencies.
  • 20 pre-existing failures in tests/gateway/test_email.py on a clean origin/main checkout, all in the sender-authentication area (TestDispatchDefersToGatewayAuthorization, authserv_id). They reproduce with this change reverted and are unrelated — flagging in case they are unexpected.

Scope note: this PR is the HTML alternative only. A cron-delivered mail also gets a generic subject and keeps the Cronjob Response: wrapper, which is a separate concern affecting other platforms and is not included here.

The email adapter sent text/plain only, so Markdown the agent writes for chat
(**bold**, ## headings, bullets) reached a mail reader as literal asterisks and
hashes. Every other platform has a Markdown strategy — Telegram converts to
MarkdownV2, Slack to mrkdwn, Matrix to HTML, SMS strips it — and email was the
only adapter with none, because email's native rich-text format is HTML rather
than Markdown.

Both send paths now attach a multipart/alternative carrying rendered HTML beside
the unchanged plain part:

- _new_reply() covers every in-process send: replies, images and documents
- _standalone_send() covers cron delivery — the sibling call path

The plain part is byte-identical to what was sent before, so a client that cannot
render HTML is unaffected. Content is escaped before any Markdown is applied, so a
body cannot inject live markup.

The converter adds no dependencies and produces inline-styled HTML (mail clients
ignore stylesheets). Matrix solves the same problem with the third-party `markdown`
library, which is not a declared Hermes dependency — it is imported there
opportunistically under suppress(ImportError) with a regex fallback. Extracting a
shared Markdown-to-HTML converter would be the better long-term shape and is
deliberately left as a follow-up rather than scope creep here.

Testing:
- tests/gateway/test_email_html_body.py (new) — 3 behaviour contracts, all proven
  red on the base by reverting the adapter: the plain part is unchanged, the HTML
  alternative renders, and untrusted markup is escaped
- tests/gateway/test_email.py — one existing test asserted msg.get_payload()[0]
  was the plain part; that index is now the alternative container, so it walks to
  the text/plain part instead. Its contract is unchanged.

Note: tests/gateway/test_email.py has 20 pre-existing failures on a clean
origin/main checkout, all in the sender-authentication area
(TestDispatchDefersToGatewayAuthorization / authserv_id). They are unrelated to
this change and reproduce with it reverted; flagging in case they are unexpected.
@daniel5simmons

Copy link
Copy Markdown
Author

Correction to the "pre-existing failures" note

My PR description says:

20 pre-existing failures in tests/gateway/test_email.py on a clean origin/main checkout …

That number is wrong, and I want to retract it rather than leave a misleading note in review.

What I measured: 20 failures in a locally bootstrapped environment I had created for this work (a fresh pm environment on Python 3.14). On a standard checkout the same file is green on pristine origin/main:

pristine origin/main adapter, standard checkout  →  0 failed, 40 passed

So those 20 failures were an artifact of my own environment, not a property of main, and I should have verified that before writing it into the description. Apologies for the noise.

For completeness, the real accounting in the standard checkout, on this branch:

with the HTML fix + updated test   →  1 failed   (the existing oversized-cron test before updating it)
with that test updated             →  0 failed

which is what the branch now carries: 0 failures in tests/gateway/test_email.py and 0 across test_email_robustness.py, test_email_charset_fallback.py, test_email_secret_scope.py, test_email_imap_id_protocol.py.

Related finding (separate, not in this PR)

While verifying the above I hit a second, unrelated bug in the same adapter: _probe_imap() baselines the mailbox with imap.uid("search", None, "ALL") and _fetch_new_messages() polls with uid("search", None, "UNSEEN"). On a mailbox with a large backlog both return every matching UID as a single response line, which exceeds imaplib's 1 MB _MAXLINE and raises got more than 1000000 bytes. Because the failing call is inside the connection probe, inbound email dies entirely rather than degrading — the adapter never becomes ready.

Reproduced against a mailbox with 319,004 messages:

UID SEARCH ALL              → got more than 1000000 bytes
UID SEARCH UNSEEN           → got more than 1000000 bytes
UID SEARCH UNSEEN UID n:*   → OK (bounded)

This is independent of this PR and I will file it separately; flagging it here only because it is the same file and reviewers may see it. Nothing in this PR touches the IMAP paths.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/email Email (IMAP/SMTP) adapter 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 Oct 14, 2026
@daniel5simmons

Copy link
Copy Markdown
Author

Correction to the correction (I got this wrong twice — sorry)

My earlier comment retracted the "20 pre-existing failures" claim, saying they were an artifact of my environment and that pristine origin/main is green. That retraction was wrong. I compared against the wrong tree.

What I actually did was measure the v0.21.3 checkout, which is where this work started, see 0 failures there, and conclude the 20 were mine. v0.21.3 is ~15,600 commits behind this PR's base. The correct comparison is against the branch the PR targets.

Measured properly, on v0.22.1+13 (the current stable, whose email adapter matches origin/main):

pristine v0.22.1+13, unmodified   →  20 failed, 43 passed
with this PR applied              →  20 failed, 43 passed

Identical counts, and the failing set is the same both ways — all in the sender-authentication area (TestDispatchDefersToGatewayAuthorization / authserv_id). So the original note in the description was accurate, and the retraction was noise I introduced.

Summary of the three states, so this is unambiguous:

Tree Result
v0.21.3 (start of this work, old branch) 0 failed — irrelevant to this PR
pristine v0.22.1+13 / origin/main 20 failed, 43 passed
this PR applied 20 failed, 43 passed

The 20 are pre-existing, unrelated to this change, and reproduce with it reverted on the current base. Apologies for the churn — I should have compared against the PR's base the first time rather than against whatever checkout happened to be in front of me.

This branch has not been deployed

No deployments
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: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 type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants