Skip to content

fix(email): key sessions and reply threading by Gmail X-GM-THRID - #99814

Open
ntg-agent wants to merge 1 commit into
NousResearch:mainfrom
ntg-agent:fix/email-per-thread-session-isolation
Open

ntg-agent wants to merge 1 commit into
NousResearch:mainfrom
ntg-agent:fix/email-per-thread-session-isolation

Conversation

@ntg-agent

Copy link
Copy Markdown

Note

This change was written by an AI agent (Claude Opus 5, via Claude Code), including this
description.
A human directed the work, supplied the requirements and approved opening the
PR, but the code, tests and prose here are AI-authored. Please review accordingly — in
particular the design decisions called out under Open questions for maintainers, which are
judgement calls rather than mechanical changes.

What does this PR do?

The email adapter never set SessionSource.thread_id, so build_session_key() collapsed all
mail from one address into a single session (agent:main:email:dm:<sender>). Two unrelated
conversations with the same person shared one context window.

The same root cause produced a second, more visible symptom: outbound In-Reply-To/References
were sourced from _thread_context[to_addr] — one slot per sender, holding "the last message
received from this address". A cron job created in thread A delivered its result into whichever
thread with that sender had been touched most recently.

This keys both the session and the outbound reply context on Gmail's server-computed
X-GM-THRID, bringing email to parity with the per-thread isolation Telegram/Discord/Slack
already have.

Related Issue

Fixes #11422

This implements Revision 2 of an internal PRD; the revision's substance — the capability-probe
reconciliation with #11422, the RFC 5322 audit, and the scope calls on #26277 / #27804 — is
reproduced inline below rather than linked, so review needs nothing external.

Relates to #26277 (same user need, subject-based mechanism — see Scope below)
Relates to #27804 (this fixes the session-isolation half; the notification-volume half is
already fixed in-tree)

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

plugins/platforms/email/adapter.py

  • Thread identity. _fetch_thread_id() issues UID FETCH (X-GM-THRID) over the existing
    imaplib connection. It runs before the RFC822 fetch, because that fetch implicitly sets
    \Seen — doing it second would strip the message of the one flag that makes a retry possible.
  • Capability probe (per feat(gateway): add session_keying=gmail_thread_id mode for email adapter #11422). _probe_gmail_ext() records whether the server advertises
    X-GM-EXT-1 once per IMAP session. Without it there is no thread id to fetch, so the adapter
    keeps the previous sender-keyed behaviour and logs one warning. Without this probe, a
    non-Gmail mailbox answers every thread lookup with BAD, burns the retry budget, and drops
    all inbound mail. Delegated mailboxes and some Workspace configs also omit the capability.
  • Bounded retry. With the capability present, a failed lookup leaves the message UNSEEN
    for the next poll cycle. After 5 consecutive failures on one UID: log under the grep-able
    prefix [Email][thread-lookup-failure] (UID and sender only — never the body), reply to the
    sender, mark seen, drop. Strike counters are pruned when a UID leaves the UNSEEN set.
  • Pipeline. thread_id flows through the existing
    build_source() → SessionSource.thread_id → build_session_key() →
    HERMES_SESSION_THREAD_ID → cron origin path. No changes to session.py or
    cronjob_tools.py
    — that logic already handled thread_id; only the email adapter failed
    to populate it.
  • Outbound threading. _thread_context rekeyed from sender_addr to
    (sender_addr, thread_id). A send naming no thread (standalone/home-channel) resolves to the
    correspondent's most recent thread; a send naming an unknown thread returns empty context
    rather than borrowing another thread's anchor — borrowing is the bug being fixed.

RFC 5322 conformance (same file)

  • §3.6.4 References — was emitting only the parent's Message-ID, discarding all earlier
    ancestry, so clients could not nest a reply in a long thread. Now builds the full chain
    (parent's References + parent's Message-ID), deduplicated, capped at 20 by keeping the
    thread root plus the most recent ancestors.
  • §3.6.4 msg-id shape — inbound Message-ID is attacker-controlled and was echoed verbatim
    into In-Reply-To/References. A value containing CRLF reached Python's header setter and
    raised HeaderParseError at send time, meaning every reply in that thread would fail
    permanently. Header injection was never possible (Python blocks it), but the availability bug
    was real. Values are now trimmed to their first well-formed <id-left@id-right> token, or
    dropped.
  • §3.6.4 uniqueness — Message-ID generation moved from 48 bits of UUID hex to
    email.utils.make_msgid().
  • §2.2 (RFC 2047 header encoding) and §2.1.1 (998-char lines) were audited and are already
    compliant; no change, but both now have regression tests.

Tests — tests/gateway/test_email_thread_isolation.py (new, 817 lines / 38 tests), plus
8 lines each in test_email.py and test_send_multiple_images.py.

Reviewer note: the two existing-test edits are not cosmetic. _thread_context is now
keyed (sender, thread_id) rather than sender, and _send_email_with_attachments takes a
thread_id argument — tests that construct or unpack either need updating. Any IMAP mock
that neither advertises X-GM-EXT-1 nor answers the X-GM-THRID fetch will exercise the
non-Gmail fallback path rather than fail, which is the intended degradation.

Scope

#27804 has two halves and they have different owners. The session-isolation half is fixed
here. The notification-volume half (100–200 status emails per request) is already fixed
in-tree
: gateway/display_config.py assigns email _TIER_MINIMAL, verified at runtime:

email.tool_progress                = 'off'
email.interim_assistant_messages   = False
email.long_running_notifications   = False
email.streaming                    = False
email.busy_ack_detail              = False

Re-implementing throttling in the adapter would duplicate working behaviour and add dead code,
so this PR adds a regression guard pinning those defaults instead, while asserting an
operator can still opt back in per-platform.

#26277 is related but not fixed. It asks for opt-in isolation by normalized subject, on any
IMAP provider. This is Gmail-only, always-on where supported, and treats a mid-thread subject
rename as the same thread (X-GM-THRID is stable) where #26277 wants it to start a new one.
Those are coherent but different products, so #26277 should stay open for non-Gmail providers.

Open questions for maintainers

  1. Opt-in vs. clean cut. feat(gateway): add session_keying=gmail_thread_id mode for email adapter #11422 specifies this behind
    platforms.email.extra.session_keying, default sender, so existing installs see zero
    change. This PR applies it automatically wherever X-GM-EXT-1 is advertised. The capability
    probe removes the correctness argument for a flag, but not the compatibility argument — on a
    Gmail mailbox, existing senders get a one-time session fragmentation (old sessions remain at
    their old keys and simply stop accumulating). Happy to add the flag if preferred.
  2. Session key shape. feat(gateway): add session_keying=gmail_thread_id mode for email adapter #11422 suggests …:dm:<addr>:gthr-<digits>; this emits bare digits.
    Trivial to change.
  3. There are five other open PRs in this area (fix(gateway): key email thread context by root Message-ID instead of sender address #11185, fix(gateway): rekey email _thread_context by (sender, message_id) to prevent reply clobber #11418, fix(email): preserve Gmail threading metadata #23999, feat(email): per-thread session routing + configurable display name #63659, fix(email): prevent thread context overwrite in concurrent conversations #73189,
    fix(email): preserve thread recipients and cursor state #90482), none merged. If a maintainer would rather see this folded into one of those, say so
    and I'll close this in favour of it.

How to Test

  1. scripts/run_tests.sh tests/gateway/test_email_thread_isolation.py tests/gateway/test_email.py tests/gateway/test_email_robustness.py tests/gateway/test_send_multiple_images.py -q
  2. Two concurrently open Gmail threads with the same sender produce two distinct session keys
    (agent:main:email:dm:<addr>:<thrid>); replying in one leaves the other's context untouched.
  3. A cron job created inside thread A and delivered with deliver="origin" nests under thread A,
    not under whichever thread was most recently touched.
  4. Against a non-Gmail IMAP server, mail is still delivered (sender-keyed), with one
    does not advertise X-GM-EXT-1 warning — this is the regression the capability probe exists
    to prevent.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate — see Open questions Architecture planning #3
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run scripts/run_tests.sh on the affected surface (1117 passed, 0 failed); whole-repo run in flight — see Logs
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04, Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings) — no user-facing docs affected
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no new config
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — N/A, no platform-specific code paths
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

The regression the capability probe prevents — simulated non-Gmail IMAP server, before vs after:

BEFORE                                  AFTER
cycle 1: delivered=0  dropped=False     delivered=1  thread_id=None
cycle 2: delivered=0  dropped=False     (sender-keyed session, mail intact)
cycle 3: delivered=0  dropped=False
cycle 4: delivered=0  dropped=False
cycle 5: delivered=0  dropped=True
error emails sent to: ['user@t.com']

RFC 5322 §3.6.4 References chain:

before  References: <p3@example.com>
after   References: <root@example.com> <p1@example.com> <p2@example.com> <p3@example.com>

Header-injecting inbound Message-ID is now sanitized rather than fatal:

<ok@x.com>\r\nBcc: attacker@evil.com   ->  In-Reply-To: <ok@x.com>   (Bcc not emitted)
not-a-valid-msgid                      ->  In-Reply-To: omitted

All figures below were measured on this commit, rebased onto current main, using
scripts/run_tests.sh (the canonical CI-equivalent runner with per-file isolation).

Email surface — the four files this touches:

tests/gateway/test_email.py
tests/gateway/test_email_robustness.py
tests/gateway/test_send_multiple_images.py
tests/gateway/test_email_thread_isolation.py     88 passed

Everything importing the changed code — 76 files referencing the email adapter,
build_session_key, DeliveryRouter or the cron scheduler:

=== Summary: 76 files, 1117 tests passed, 0 failed, 2 skipped in 140.2s (8 workers) ===

In flight: the whole-repo scripts/run_tests.sh run on this rebased commit is still
executing at the time of opening; I'll post the result as a comment. An earlier whole-repo run
of the same change against an older base gave 39,210 passed / 10 failed, where all 10 were
pre-existing flakes verified by stashing the change and re-running each (mostly
mtime-granularity cache-invalidation tests — e.g. test_skill_utils.py failed 3/6 baseline
runs and 5/6 with the change). That run is not this commit and is offered only as context.

Note: the project's declared dev extra was incomplete in my environment — pytest-asyncio was
missing, which fails every @pytest.mark.asyncio test repo-wide. Installing the [dev] pins
from pyproject.toml was required to get a meaningful run.

The email adapter never set SessionSource.thread_id, so build_session_key()
collapsed all mail from one address into a single session. Two unrelated
conversations with the same person shared one context window.

The same root cause drove a second symptom: outbound In-Reply-To/References
came from _thread_context[to_addr] — one slot per sender holding "the last
message received from this address" — so a cron job created in thread A
delivered its result into whichever thread with that sender was most
recently touched.

Key both the session and the outbound reply context on Gmail's
server-computed X-GM-THRID:

- Fetch X-GM-THRID before the RFC822 fetch, which implicitly sets \Seen and
  would otherwise strip the flag that makes a retry possible.
- Probe X-GM-EXT-1 once per IMAP session. Without the extension there is no
  thread id to fetch, so keep the previous sender-keyed behaviour and warn
  once. Without this probe a non-Gmail mailbox answers every lookup with BAD,
  burns the retry budget and drops all inbound mail.
- Bound the retry: 5 consecutive failures on one UID logs under a grep-able
  prefix (UID and sender only, never the body), replies to the sender, marks
  seen and drops.
- Rekey _thread_context from sender_addr to (sender_addr, thread_id).

Also bring the outbound path into RFC 5322 conformance:

- §3.6.4 References now carries the parent's chain plus the parent, instead
  of only the parent's Message-ID, so clients can nest deep threads.
- §3.6.4 msg-id values from inbound mail are validated before being echoed.
  A CRLF-bearing Message-ID previously raised HeaderParseError at send time,
  making every reply in that thread fail permanently.
- §3.6.4 uniqueness: Message-ID generation moves to email.utils.make_msgid().

thread_id flows through the existing build_source -> SessionSource ->
build_session_key -> HERMES_SESSION_THREAD_ID -> cron origin pipeline;
session.py and cronjob_tools.py are unchanged.

Fixes NousResearch#11422
Relates to NousResearch#26277
Relates to NousResearch#27804

This change was written by an AI agent (Claude Opus 5, via Claude Code),
under human direction and with human approval to submit it.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/email Email (IMAP/SMTP) adapter sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 31, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related to #11418, #11422, and #23999: this broader email patch combines Gmail thread-scoped session isolation with reply-thread header handling. It is a competing superset rather than a duplicate of any single open PR.

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 P3 Low — cosmetic, nice to have platform/email Email (IMAP/SMTP) adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants