fix(email): send HTML bodies as multipart/alternative

gateway/platforms/email.py

@@ -17,6 +17,7 @@
 
 import asyncio
 import email as email_lib
+import html as html_lib
 import imaplib
 import logging
 import os
@@ -161,10 +162,59 @@ def _extract_text_body(msg: email_lib.message.Message) -> str:
 
 
 def _strip_html(html: str) -> str:
-    """Naive HTML tag stripper for fallback text extraction."""
-    text = re.sub(r"<br\s*/?>", "\n", html, flags=re.IGNORECASE)
+    """Convert an HTML fragment to readable plain text.
+
+    Used for the inbound text extraction *and* the outbound ``text/plain``
+    alternative of an HTML body, so it keeps the parts that matter without
+    markup: link targets become ``label (url)`` so URLs survive, and block/list
+    boundaries become newlines so lists and tables don't collapse into one run.
+    """
+    # Drop <style>/<script> blocks entirely (tag *and* contents) so embedded
+    # CSS/JS from full HTML documents doesn't leak into the text fallback.
+    text = re.sub(
+        r"<(style|script)\b[^>]*>.*?</\1\s*>",
+        "",
+        html,
+        flags=re.IGNORECASE | re.DOTALL,
+    )
+    # Preserve link targets: <a href="url">label</a> -> "label (url)".
+    def _anchor(m: re.Match) -> str:
+        url = (m.group("url") or "").strip()
+        label = _strip_html(m.group("label")).strip()
+        if not url or url == label:
+            return label
+        return f"{label} ({url})" if label else url
+
+    text = re.sub(
+        r"""<a\b[^>]*\bhref\s*=\s*["']?(?P<url>[^"'>\s]+)["']?[^>]*>(?P<label>.*?)</a>""",
+        _anchor,
+        text,
+        flags=re.IGNORECASE | re.DOTALL,
+    )
+    # Preserve images: <img src="url" alt="text"> -> "[image: alt (url)]" so a
+    # text-only / image-only body isn't blank.
+    def _img(m: re.Match) -> str:
+        tag = m.group(0)
+        src = re.search(r"""\bsrc\s*=\s*["']?([^"'>\s]+)""", tag, re.IGNORECASE)
+        alt = re.search(r"""\balt\s*=\s*["']([^"']*)["']""", tag, re.IGNORECASE)
+        parts = [p for p in (alt.group(1).strip() if alt else "",
+                             src.group(1).strip() if src else "") if p]
+        return f"[image: {' '.join(parts)}]" if parts else "[image]"
+
+    text = re.sub(r"<img\b[^>]*>", _img, text, flags=re.IGNORECASE)
+    # Line breaks for explicit breaks and block/list/row boundaries so digests
+    # stay readable (<ul><li>One</li><li>Two</li></ul> -> "One\nTwo").
+    text = re.sub(r"<br\s*/?>", "\n", text, flags=re.IGNORECASE)
     text = re.sub(r"<p[^>]*>", "\n", text, flags=re.IGNORECASE)
-    text = re.sub(r"</p>", "\n", text, flags=re.IGNORECASE)
+    text = re.sub(
+        r"</(?:p|div|li|ul|ol|tr|table|h[1-6]|blockquote)\s*>",
+        "\n",
+        text,
+        flags=re.IGNORECASE,
+    )
+    text = re.sub(r"<li[^>]*>", "\n", text, flags=re.IGNORECASE)
+    # Tab between table cells so columns don't fuse ("A</td><td>B" -> "A\tB").
+    text = re.sub(r"</(?:td|th)\s*>\s*<(?:td|th)[^>]*>", "\t", text, flags=re.IGNORECASE)
     text = re.sub(r"<[^>]+>", "", text)
     text = re.sub(r"&nbsp;", " ", text)
     text = re.sub(r"&amp;", "&", text)
@@ -174,6 +224,78 @@ def _strip_html(html: str) -> str:
     return text.strip()
 
 
+# Whether a body *already* contains HTML markup we should send as-is, versus
+# plain text we should HTML-escape before wrapping. This no longer gates whether
+# HTML is sent (we always send a multipart/alternative) — it only decides if the
+# HTML part is the body verbatim or an escaped+linebreaked version, and how the
+# plain part is built. Detection requires a *real* tag terminator so prose
+# comparisons against tag-named variables (``if x<div and div>0``, ``x<p``,
+# ``x<a and a>0``) don't get misread as markup:
+#   * Any listed tag matches when immediately closed (``<div>``), self-closed
+#     (``<br/>``), or followed by an attribute (``<div class=...>``,
+#     ``<img src=...>``) — i.e. ``<name`` then whitespace then ``word=``.
+#   * A bare ``<name `` followed by a non-attribute word (``<div and``) does NOT
+#     match, since that's how comparisons read.
+#   * ``a`` additionally needs ``href=``/``name=`` or an actual ``<a>``/``</a>``.
+#   * Any well-formed *closing* tag (``</p>``) also counts.
+_HTML_TAGS = r"html|body|div|br|h[1-6]|ul|ol|li|table|tr|td|th|span|strong|em|b|i|p|img|pre|code|blockquote"
+_HTML_CLOSE_TAGS = r"html|body|div|p|h[1-6]|a|ul|ol|li|table|tr|td|th|span|strong|em|b|i|pre|code|blockquote"
+_HTML_BODY_RE = re.compile(
+    rf"<\s*(?:{_HTML_TAGS})\s*/?>"                 # <tag> or <tag/>
+    rf"|<\s*(?:{_HTML_TAGS})\s+[a-zA-Z-]+\s*=" # <tag attr=...> (real attribute)
+    rf"|<\s*a\s+(?:href|name)\b"                   # anchor with a real attribute …
+    rf"|<\s*a\s*>|<\s*/\s*a\s*>"                   # … or an actual <a>/</a> tag
+    rf"|<\s*/\s*(?:{_HTML_CLOSE_TAGS})\s*>",       # any closing tag
+    re.IGNORECASE,
+)
+
+
+def _is_html_body(body: str) -> bool:
+    """Heuristic: is this body already HTML markup (vs. plain text to escape)?"""
+    return bool(_HTML_BODY_RE.search(body))
+
+
+def _text_to_html(text: str) -> str:
+    """Convert a plain-text body to a safe HTML fragment.
+
+    Escapes ``< > &`` so literal markup in prose (e.g. a coding-help reply that
+    mentions ``<div class="card">``) is shown as text rather than rendered, and
+    wraps the result in a ``white-space: pre-wrap`` block so indentation/
+    alignment survives (logs, code, tables) *and* long lines/URLs still wrap
+    instead of being clipped — which a bare ``<pre>`` would prevent.
+    """
+    return (
+        f'<pre style="white-space: pre-wrap; word-wrap: break-word;">'
+        f"{html_lib.escape(text)}</pre>"
+    )
+
+
+def _attach_body(msg: MIMEMultipart, body: str) -> None:
+    """Attach *body* to *msg* as a ``multipart/alternative``.
+
+    Always carries two parts so we never have to guess *whether* to send HTML —
+    only how to build each part:
+
+    - ``text/plain``: the readable text. For a plain body that's the body
+      verbatim; for an HTML body it's the tag-stripped text (so text-only
+      clients and indexed snippets don't see raw ``<h2>``/``<p>`` markup).
+    - ``text/html``: an HTML body verbatim, or a plain body escaped and wrapped
+      in ``<pre>`` via :func:`_text_to_html`.
+
+    Either way the plain part is always readable and misdetection can't regress
+    an email below plain text.
+    """
+    if not body:
+        return
+    is_html = _is_html_body(body)
+    plain_part = _strip_html(body) if is_html else body
+    html_part = body if is_html else _text_to_html(body)
+    alt = MIMEMultipart("alternative")
+    alt.attach(MIMEText(plain_part, "plain", "utf-8"))
+    alt.attach(MIMEText(html_part, "html", "utf-8"))
+    msg.attach(alt)
+
+
 def _extract_email_address(raw: str) -> str:
     """Extract bare email address from 'Name <addr>' format."""
     match = re.search(r"<([^>]+)>", raw)
@@ -546,7 +668,7 @@ def _send_email(
         msg_id = f"<hermes-{uuid.uuid4().hex[:12]}@{self._address.split('@')[1]}>"
         msg["Message-ID"] = msg_id
 
-        msg.attach(MIMEText(body, "plain", "utf-8"))
+        _attach_body(msg, body)
 
         smtp = smtplib.SMTP(self._smtp_host, self._smtp_port, timeout=30)
         try:
@@ -655,8 +777,7 @@ def _send_email_with_attachments(
         msg_id = f"<hermes-{uuid.uuid4().hex[:12]}@{self._address.split('@')[1]}>"
         msg["Message-ID"] = msg_id
 
-        if body:
-            msg.attach(MIMEText(body, "plain", "utf-8"))
+        _attach_body(msg, body)
 
         for file_path in file_paths:
             p = Path(file_path)
@@ -736,8 +857,7 @@ def _send_email_with_attachment(
         msg_id = f"<hermes-{uuid.uuid4().hex[:12]}@{self._address.split('@')[1]}>"
         msg["Message-ID"] = msg_id
 
-        if body:
-            msg.attach(MIMEText(body, "plain", "utf-8"))
+        _attach_body(msg, body)
 
         # Attach file
         p = Path(file_path)

tests/gateway/test_email.py

@@ -147,6 +147,172 @@ def test_strip_html_entities(self):
         result = _strip_html(html)
         self.assertIn("a & b", result)
 
+    def test_strip_html_preserves_link_url(self):
+        # Codex feedback: text-only readers must keep the actual URL.
+        from gateway.platforms.email import _strip_html
+        result = _strip_html('See <a href="https://x.com/report">the report</a>')
+        self.assertIn("the report", result)
+        self.assertIn("https://x.com/report", result)
+
+    def test_strip_html_keeps_list_boundaries(self):
+        # Codex feedback: lists must not collapse into one run.
+        from gateway.platforms.email import _strip_html
+        result = _strip_html("<ul><li>One</li><li>Two</li></ul>")
+        self.assertIn("One", result)
+        self.assertIn("Two", result)
+        self.assertNotIn("OneTwo", result.replace("\n", "X"))  # separated, not fused
+
+    def test_strip_html_keeps_table_row_boundaries(self):
+        from gateway.platforms.email import _strip_html
+        result = _strip_html("<table><tr>R1</tr><tr>R2</tr></table>")
+        self.assertNotIn("R1R2", result.replace("\n", "X"))
+
+    def test_strip_html_separates_table_cells(self):
+        # Codex feedback: <td>/<th> must not fuse columns.
+        from gateway.platforms.email import _strip_html
+        result = _strip_html("<tr><td>A</td><td>B</td></tr>")
+        self.assertNotIn("AB", result.replace("\t", "X").replace("\n", "X"))
+        self.assertIn("A", result)
+        self.assertIn("B", result)
+
+    def test_strip_html_drops_style_and_script(self):
+        # Codex feedback: embedded CSS/JS must not leak into the text fallback.
+        from gateway.platforms.email import _strip_html
+        result = _strip_html("<style>.x{color:red}</style><h1>Digest</h1>"
+                             "<script>alert(1)</script>")
+        self.assertEqual(result.strip(), "Digest")
+        self.assertNotIn("color:red", result)
+        self.assertNotIn("alert", result)
+
+    def test_strip_html_preserves_image_reference(self):
+        # Codex feedback: image-only body must not become blank.
+        from gateway.platforms.email import _strip_html
+        result = _strip_html('<img src="https://e.com/c.png" alt="chart">')
+        self.assertIn("chart", result)
+        self.assertIn("https://e.com/c.png", result)
+
+
+class TestHtmlBodyDetection(unittest.TestCase):
+    """Every email is multipart/alternative; detection only shapes the HTML part."""
+
+    @staticmethod
+    def _parts(msg):
+        """Return {content_type: payload_str} for the alternative subparts."""
+        alt = msg.get_payload()[0]
+        return {sub.get_content_type(): sub.get_payload(decode=True).decode("utf-8")
+                for sub in alt.get_payload()}
+
+    def test_detects_html_body(self):
+        from gateway.platforms.email import _is_html_body
+        self.assertTrue(_is_html_body("<h2>Digest</h2><p>Item</p>"))
+        self.assertTrue(_is_html_body('Read <a href="http://x">more</a>'))
+
+    def test_detects_single_letter_tags(self):
+        from gateway.platforms.email import _is_html_body
+        # Real single-letter tags must still be detected as HTML.
+        self.assertTrue(_is_html_body("Hello <b>world</b>"))
+        self.assertTrue(_is_html_body("Hello <i>world</i>"))
+        self.assertTrue(_is_html_body("Line one<br/>Line two"))
+
+    def test_detects_standalone_tags(self):
+        from gateway.platforms.email import _is_html_body
+        # Codex feedback: image-only / preformatted bodies are valid HTML too.
+        self.assertTrue(_is_html_body('<img src="http://x/y.png">'))
+        self.assertTrue(_is_html_body("<pre>code block</pre>"))
+
+    def test_plain_text_not_detected_as_html(self):
+        from gateway.platforms.email import _is_html_body
+        self.assertFalse(_is_html_body("Plain text, no markup."))
+        self.assertFalse(_is_html_body("if x < y and y > z: pass"))
+        self.assertFalse(_is_html_body("I <3 this"))
+        # Comparisons against single-letter operands must not look like <b>/<i>/<p>.
+        self.assertFalse(_is_html_body("a<b and b>c"))
+        self.assertFalse(_is_html_body("5<i means five is less than i"))
+        self.assertFalse(_is_html_body("if x<p and p>0: pass"))
+        # Comparisons against multi-letter tag-named variables must not match
+        # either: a real tag closes (<div>) or has an attribute (<div class=),
+        # but "<div and" / "<code and" is a comparison.
+        self.assertFalse(_is_html_body("if x<div and div>0: pass"))
+        self.assertFalse(_is_html_body("if x<code and code>0: pass"))
+        self.assertFalse(_is_html_body("if x<span and span>0: pass"))
+
+    def test_detects_tags_with_attributes(self):
+        from gateway.platforms.email import _is_html_body
+        self.assertTrue(_is_html_body('<div class="card">x</div>'))
+        self.assertTrue(_is_html_body('<img src="http://x/y.png">'))
+
+    def test_attach_always_multipart_alternative(self):
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body
+        for body in ("<p>Hello <strong>world</strong></p>", "Just plain text"):
+            msg = MIMEMultipart()
+            _attach_body(msg, body)
+            self.assertEqual(sorted(self._parts(msg)), ["text/html", "text/plain"])
+
+    def test_html_body_verbatim_in_html_part_stripped_in_plain(self):
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body
+        msg = MIMEMultipart()
+        _attach_body(msg, "<h2>Digest</h2><p>Hi <strong>world</strong></p>")
+        parts = self._parts(msg)
+        # HTML part keeps the markup verbatim …
+        self.assertIn("<strong>world</strong>", parts["text/html"])
+        # … but the plain part is tag-stripped, so text-only clients and indexed
+        # snippets never see raw <h2>/<p> markup (Codex feedback).
+        self.assertNotIn("<p>", parts["text/plain"])
+        self.assertNotIn("<h2>", parts["text/plain"])
+        self.assertIn("Digest", parts["text/plain"])
+        self.assertIn("world", parts["text/plain"])
+
+    def test_plain_prose_kept_verbatim_in_plain_part(self):
+        # A reply with no *complete* tag is plain text: plain part is verbatim,
+        # HTML part escapes it so the brackets show as text rather than render.
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body
+        msg = MIMEMultipart()
+        body = "Compare a<b to verify the sort order"
+        _attach_body(msg, body)
+        parts = self._parts(msg)
+        self.assertEqual(parts["text/plain"], body)
+        self.assertIn("a&lt;b", parts["text/html"])         # escaped, shown as text
+
+    def test_plain_text_whitespace_preserved_and_wraps(self):
+        # Codex feedback: indentation/alignment must survive (logs, code, tables)
+        # AND long lines must still wrap — so it's <pre> with white-space:pre-wrap,
+        # not a bare <pre> that disables wrapping.
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body
+        msg = MIMEMultipart()
+        _attach_body(msg, "col1    col2\n  indented line")
+        html = self._parts(msg)["text/html"]
+        self.assertIn("<pre", html)
+        self.assertIn("pre-wrap", html)                      # long lines still wrap
+        self.assertIn("col1    col2", html)                  # repeated spaces kept
+
+    def test_anchor_comparison_not_treated_as_html(self):
+        # Codex feedback: "if x<a and a>0" must NOT be parsed as an <a> anchor.
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body, _is_html_body
+        body = "if x<a and a>0: pass"
+        self.assertFalse(_is_html_body(body))
+        msg = MIMEMultipart()
+        _attach_body(msg, body)
+        parts = self._parts(msg)
+        self.assertEqual(parts["text/plain"], body)          # sent as plain, verbatim
+        self.assertNotIn("<a", parts["text/html"])           # escaped, not a tag
+
+    def test_real_anchor_still_detected(self):
+        from gateway.platforms.email import _is_html_body
+        self.assertTrue(_is_html_body('Click <a href="http://x">here</a>'))
+        self.assertTrue(_is_html_body("Link: <a>text</a>"))
+
+    def test_attach_empty_body_is_noop(self):
+        from email.mime.multipart import MIMEMultipart
+        from gateway.platforms.email import _attach_body
+        msg = MIMEMultipart()
+        _attach_body(msg, "")
+        self.assertEqual(msg.get_payload(), [])
+
 
 class TestExtractTextBody(unittest.TestCase):
     """Test email body extraction from different message formats."""