Skip to content

fix(email): avoid double-decoding HTML entities - #68707

Open
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/email-html-entity-double-decode
Open

fix(email): avoid double-decoding HTML entities#68707
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/email-html-entity-double-decode

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents the email HTML fallback from decoding two entity layers in one pass. Nested escaped text such as &amp;lt;APIKEY&amp;gt; now remains literal &lt;APIKEY&gt; instead of becoming <APIKEY>.

Related Issue

Fixes #68704

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Tests (adding or improving test coverage)

Changes Made

  • Decode &lt; and &gt; before &amp; in plugins/platforms/email/adapter.py.
  • Add regression coverage for nested escaped entities in tests/gateway/test_email.py.

How to Test

  1. Run pytest tests/gateway/test_email.py -q -k strip_html.
  2. Confirm all four HTML stripping tests pass.
  3. Confirm _strip_html("The token is &amp;lt;APIKEY&amp;gt;") returns The token is &lt;APIKEY&gt;.

Checklist

Code

  • I've read the Contributing Guide
  • My commit message follows Conventional Commits
  • I searched for existing PRs and issues to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on Windows 11

Documentation & Housekeeping

  • Documentation update: N/A
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md / AGENTS.md: N/A
  • Cross-platform impact considered
  • Tool descriptions/schemas: N/A

Screenshots / Logs

Focused regression suite:

4 passed, 85 deselected

Ruff passed for both touched files. A whole-file run reached one unrelated existing Windows environment failure in TestConfigEnvOverrides.test_email_not_loaded_without_env: clearing all environment variables makes Path.home() unable to resolve. The focused behavior covered by this PR passes.

@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/email Email (IMAP/SMTP) adapter P3 Low — cosmetic, nice to have labels Jul 21, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. Current main still decodes &amp; before &lt;/&gt; in plugins/platforms/email/adapter.py:231-233, so nested escaped entities are decoded twice in one _strip_html() call. The proposed ordering directly prevents that, and the added test covers the exact reported input.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026
@swissly

swissly commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Overlap note (2026-08-03): this PR addresses the same topic (HTML email delivery). PR #73294 (swissly) consolidates Markdown-to-HTML rendering for ALL 4 send paths + allowlist sanitizer + 20 tests, and was just rebased onto current main (commit 9529ee30). Please review #73294 for consolidation rather than duplicating send-path fixes.

@swissly

swissly commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Coordination note from #73294 (swissly, 2026-08-03):

Agreed — #68707 is the correct, minimal fix for #68704 (decode &lt;/&gt; before &amp; so each entity layer decodes at most once). I verified the reproduction locally and applied the identical fix to my deployed main (commit 73c0f09) with your regression test, since the bug was live on my gateway.

Scope clarification: #73294 (Markdown-to-HTML rendering) touches the SEND paths (_markdown_to_html_email, _attach_parts, _standalone_send) — it does NOT touch _strip_html (receive/fallback path). The two PRs are complementary, not duplicative: #73294 makes outbound emails HTML, #68707 fixes inbound HTML-to-text decoding. Neither needs to absorb the other.

Recommendation to maintainers: #68707 can merge independently of #73294.

@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses issue #68704. The diff of #68707 changes the entity-decoding order in _strip_html() so nested escaped entities lose only one layer per call and adds regression coverage for the exact reported input.

Related pull requests

Suggested consolidation

Keep #68707 open with a salvage path: retain its minimal decoding-order correction and focused regression test for #68704. No supplied PR is a duplicate; specifically, the discussion identifies #73294 as complementary outbound-email work rather than an alternative fix for the receive/fallback path.

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
    I68704(["issue #68704 (open)"])
    P68707["PR #68707 (open)"]
    P68707 -->|best fix| I68704
    class I68704 open
    class P68707 open
    class P68707 best
    class P68707 target
    click I68704 "https://github.com/NousResearch/hermes-agent/issues/68704"
    click P68707 "https://github.com/NousResearch/hermes-agent/pull/68707"
Loading

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 (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 1 kB of PR diffs, 3 kB of issue/PR text, 1 kB of discussion (2 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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.
@ooiuuii
ooiuuii force-pushed the fix/email-html-entity-double-decode branch from 3ba56f8 to 2054488 Compare August 13, 2026 04:50
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 P3 Low — cosmetic, nice to have platform/email Email (IMAP/SMTP) adapter sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Email HTML fallback double-decodes escaped entities

5 participants