fix(email): preserve long cron report delivery - #53264
Conversation
Related: #27680 (canonical open PR for implicit-TLS SMTPS/465) and the already-merged #46084 (SMTP_SSL-465 + IPv4 fallback in the old |
2598775 to
f50ef12
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the cron-delivery problem. The current-main premise is real: gateway/delivery.py:430-451 truncates Email output because EmailAdapter has no capability flag.
Problems
plugins/platforms/email/adapter.py:428declaressplits_long_messages=True, butEmailAdapter.send()forwards one unchanged body to_send_email()(plugins/platforms/email/adapter.py:893-905), which creates oneMIMETextbody (:949).gateway/delivery.py:409-412defines this flag as native adapter chunking, so this bypasses the guard without providing the advertised bound.tests/gateway/test_email.py:270-273only asserts the flag; it does not exercise delivery routing or SMTP output.
Suggested changes
- Implement bounded native email chunking, or use a distinct capability for direct long-body transports.
- Add a DeliveryRouter → EmailAdapter regression that proves a >4K report reaches the SMTP path without the truncation footer.
Automated hermes-sweeper review.
| # Email can carry report-sized plaintext bodies directly. Mark it as a | ||
| # long-message-preserving adapter so cron/live delivery does not apply the | ||
| # 4K chat-platform truncation guard before calling send(). | ||
| splits_long_messages = True |
There was a problem hiding this comment.
gateway/delivery.py:409-412 treats this flag as a promise that send() chunks the full payload. EmailAdapter currently sends one unchanged MIMEText body (send() → _send_email()), so please either implement bounded native chunking here or use a distinct capability for direct long-body email delivery.
f50ef12 to
d6cd777
Compare
|
Addressed the sweeper findings in the current head |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two open PRs address the same long-email truncation cause: #53264 adds a transport-specific preservation capability to the delivery router, while #68900 reuses the native-chunking capability even though Email sends one intact MIME body rather than splitting it.
Related pull requests
- #53264
related— (+39/-2) — preferred, but coverage still incomplete: the diff introducespreserves_long_messages, routes full bodies past the 4K chat truncation guard, and avoids falsely claiming native chunking. The keep_open review on #53264 is only partially addressed: the regression test proves the full body reachesEmailAdapter.send(), but because that method is monkeypatched, it does not prove the body reaches_send_email()or the SMTP/MIME path unchanged. - #68900
duplicate— (+14/-0) — duplicate with an inaccurate capability contract: it setssplits_long_messages=Truealthough the adapter does not split messages, and its test only asserts that flag rather than exercising delivery routing or SMTP output. The contributor discussion also identifies it as a duplicate of earlier #62000.
Duplicates
#68900 substantially duplicates the long-email preservation fix pursued by #53264 and was also identified in contributor discussion as a duplicate of #62000.
Suggested consolidation
Merge #53264 after adding a regression that exercises DeliveryRouter through the real EmailAdapter.send() to the mocked SMTP/_send_email() boundary and verifies the >4K body remains unchanged; this preserves the more accurate transport capability while fully addressing the keep_open review. Close #68900 as a duplicate because its splits_long_messages flag misrepresents the adapter behavior and provides weaker coverage.
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 Dup53264 ["PRs duplicating each other"]
P53264["PR #53264 (open)"]
P68900["PR #68900 (open)"]
end
class P53264 open
class P68900 open
class P53264 target
click P53264 "https://github.com/NousResearch/hermes-agent/pull/53264"
click P68900 "https://github.com/NousResearch/hermes-agent/pull/68900"
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: 5 kB of PR diffs, <1 kB of issue/PR text, 2 kB of discussion (4 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
d6cd777 to
0a336fe
Compare
|
Addressed the latest cross-PR review in head |
0a336fe to
f089e29
Compare
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Delta since our previous triage comment
@petrakersten’s update corrects the coverage gap identified in our previous comment: #53264 now keeps the real EmailAdapter.send() path and mocks only _send_email, and the diff verifies that a >4K body reaches that boundary unchanged. @teknium1 also supplied contributor-level confirmation that #68900 still misuses the native-chunking contract and lacks equivalent router-to-email coverage.
Changed pull requests
- #53264
related— (+131/-2) — preferred, prior condition now satisfied: the current diff adds the distinctpreserves_long_messagescapability and exercises DeliveryRouter through the realEmailAdapter.send()path to mocked_send_email, explicitly addressing the keep_open review on #53264. - #68900
duplicate— (+244/-1) — duplicate with an incorrect capability contract: despite the keep_open review on #68900, its diff still declaressplits_long_messages=Truewithout implementing bounded splitting and tests only the flag; @teknium1’s contributor review confirms that mismatch, while #53264 implements the suggested distinct capability and end-to-end boundary regression.
Suggested consolidation
The recommendation advances from conditional to merge #53264 and close #68900 as a duplicate because the required regression is now present in #53264’s diff, notwithstanding the current backlog-lane classification.
Complex graph unchanged since our previous triage comment.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 19 kB of PR diffs, <1 kB of issue/PR text, 4 kB of discussion (7 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
f089e29 to
07417ba
Compare
|
Rebased onto current |
07417ba to
4c72208
Compare
|
Maintainer check-in: the branch is narrow, rebased, and the previous review findings have been addressed. We are reducing our actively maintained upstream queue and do not want to keep refreshing P3 contributions without a maintainer signal. If preserving long email delivery is useful upstream, we are happy to respond to review or reshape it. If there is no maintainer interest or direction by 3 September 2026, we will close it and retain the local mitigation. |
Summary
send().Test Plan
scripts/run_tests.sh tests/gateway/test_email.py -q