Skip to content

fix(gateway): add IMAP ID extension and SMTP port 465 support - #13564

Open
BarryLee wants to merge 1 commit into
NousResearch:mainfrom
BarryLee:fix/email-163-imap-id
Open

fix(gateway): add IMAP ID extension and SMTP port 465 support#13564
BarryLee wants to merge 1 commit into
NousResearch:mainfrom
BarryLee:fix/email-163-imap-id

Conversation

@BarryLee

Copy link
Copy Markdown

What does this PR do?

  • Add RFC 2971 IMAP ID extension support for providers that require client identification (notably 163.com)
  • Fix SMTP connection to properly handle implicit TLS on port 465 per RFC 8314

Platforms Tested

  • macOS (local development)
  • Raspberry Pi 4B (Docker container)

Notes

  • The IMAP ID command is sent after every login to ensure compatibility
  • ID command failures are logged but don't fail the connection (it's optional per RFC)

Related Issue

#13558

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

IMAP ID Extension (RFC 2971)

Some mail servers (notably 163.com) reject connections as "Unsafe Login" if the client doesn't identify itself via the IMAP ID command after login. This PR adds _send_imap_id() which:

  • Checks if the server advertises ID capability
  • Sends a minimal client identity
  • Gracefully skips if ID is not supported (per RFC, it's optional)

SMTP Connection Fix (RFC 8314)

Previously, the code always used STARTTLS regardless of port. Per RFC 8314:

  • Port 465 should use implicit TLS (SMTP_SSL)
  • Port 587 should use STARTTLS

This fixes connection issues with providers like 163.com that use port 465 for SMTP.

How to Test

  1. Setup 163.com email in hermes setup gateway
  2. Add EMAIL_SMTP_PORT=465 to ~/.hermes/.env
  3. IMAP login now succeeds (previously failed with "Unsafe Login"); SMTP send works correctly on port 465
  4. All existing email tests pass in tests/gateway/test_email.py

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
    • Some (82) tests failed, all unrelated to this change
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform:

Documentation & Housekeeping

  • [N/A] I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • [N/A] I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • [N/A] I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • [N/A] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

  • This skill is broadly useful to most users (if bundled) — see Contributing Guide
  • SKILL.md follows the standard format (frontmatter, trigger conditions, steps, pitfalls)
  • No external dependencies that aren't already available (prefer stdlib, curl, existing Hermes tools)
  • I've tested the skill end-to-end: hermes --toolsets skills -q "Use the X skill to do Y"

Screenshots / Logs

- Add RFC 2971 IMAP ID extension for providers requiring client identification
- Fix SMTP connection to handle implicit TLS on port 465 per RFC 8314
- Improve error logging with logger.exception() for better debugging
@Sanjays2402

Copy link
Copy Markdown
Contributor

Looks good — clean separation between _send_imap_id() (capability-gated, no-op on Gmail/Outlook) and the dedicated _create_smtp_connection() for implicit TLS on 465. RFC 2971 + RFC 8314 both honored.

Two small things worth tightening before merge:

  • The IMAP ID failure path swallows the exception silently — a logger.debug() (not warn, since it's optional per RFC) with the server's response would help future debugging on quirky providers.
  • The 465 vs 587 branch decides on port number alone. Worth a config knob (e.g. smtp_implicit_tls: true) for the rare case someone runs implicit TLS on a non-standard port (some corporate mail setups do this).

Otherwise — ship it. Happy to ❤️/approve once those land.

@Sanjays2402

Copy link
Copy Markdown
Contributor

Hey @BarryLee — heads up, the landscape shifted on this one yesterday. Wanted to flag it so your work doesn't get lost.

IMAP ID half (lines 32–105 of this PR)

#22796 landed on main yesterday (2026-05-09) and adds an _send_imap_id() of its own using imap.xatom("ID", ...). So that part of your PR is now effectively a no-op against main.

Worth noting your version is technically more careful than the merged one in two ways:

  1. Capability gating via imap.capability() so we only send ID if the server advertises the extension. The merged version sends it unconditionally and swallows errors — works, but slightly noisier on servers that don't care for unknown verbs.
  2. Bypasses imaplib._command() by writing the tagged line directly. The merged version uses xatom, which goes through _command()'s argument tokenizer — that's been a footgun on some imaplib versions for parenthesized list args, but it happens to work in current cpython.

Not arguing for a revert — xatom works and the merged version is shorter — just noting that if you rebase, the second _send_imap_id() definition will collide and you'll want to drop yours.

SMTP port 465 half (lines 100–113 + 4 call sites) — this is the part worth keeping

This is genuinely still broken on main and #22796 didn't address it. I just dumped gateway/platforms/email.py@main and counted: 4 call sites still do smtplib.SMTP(host, port) followed by starttls(...) unconditionally:

  • gateway/platforms/email.py:319 (connect/test)
  • gateway/platforms/email.py:551 (_send_email)
  • gateway/platforms/email.py:673 (reply path)
  • gateway/platforms/email.py:752 (notification path)

For 163.com's SMTP on 465, that breaks per RFC 8314 — port 465 is implicit TLS, so the right move is smtplib.SMTP_SSL(host, 465, ...) from the start; STARTTLS on top throws smtplib.SMTPNotSupportedError (or worse, hangs).

Your _create_smtp_connection(host, port) helper that picks SMTP_SSL for 465 / SMTP+starttls for 587 is exactly the right shape, and the test you added covers it.

Suggested path forward

Two options, your call:

  1. Rebase + scope down. Drop the IMAP ID definition + its two call-site additions (since fix(email): send IMAP ID extension to support 163/NetEase mailbox (salvage #22528) #22796 already did that), keep only the SMTP-465 helper + the 4 call-site swaps + the SMTP test. Becomes a clean ~40-line focused fix.
  2. Close this and open a new narrow PR for just the SMTP-465 fix.

Either works. I'd lean toward (1) since the test scaffolding and call-site changes are already done — less churn for maintainers to review.

Happy to help rebase or pull out the SMTP commit if useful — just ping. Thanks for digging in on this one. 🍰

@teknium1

Copy link
Copy Markdown
Contributor

Thank you for the original RFC 2971/RFC 8314 investigation. Current main has already incorporated the normal-adapter portions: IMAP ID is implemented at plugins/platforms/email/adapter.py:127-147 and called after both logins (:585-587, :654-658); SMTP port 465 uses SMTP_SSL through _connect_smtp() (:509-549). Those landed in 3fd4ccbd8 and 04d4471d.

Problems

  • The submitted diff edits the former gateway/platforms/email.py, which was migrated to plugins/platforms/email/adapter.py in 560010547; it cannot apply directly to current main.
  • One current sibling remains: registered standalone delivery (standalone_sender_fn at plugins/platforms/email/adapter.py:1268) still does smtplib.SMTP(...); starttls() unconditionally at :1225-1226, including port 465. Existing standalone coverage tests only port 587 at tests/gateway/test_email.py:1203-1233.

Suggested changes

  • Preserve the already-landed adapter behavior and carry the remaining port-465 handling into _standalone_send, with a focused SMTP_SSL/no-STARTTLS regression test.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants