Skip to content

fix(gateway): preserve email attachment boundaries - #72674

Open
HeyBobChan wants to merge 1 commit into
NousResearch:mainfrom
HeyBobChan:codex/email-media-fifo
Open

HeyBobChan wants to merge 1 commit into
NousResearch:mainfrom
HeyBobChan:codex/email-media-fifo

Conversation

@HeyBobChan

Copy link
Copy Markdown

What does this PR do?

Preserves native message boundaries for adapters such as Email whose media-bearing events are already complete messages.

Before this change, two emails arriving from the same sender while a turn was busy could be merged into one pending event. The merged event retained the first email's Message-ID and source while appending the later email's text and attachments. A downstream hook therefore could not determine which attachment belonged to which RFC message.

This adds a generic, default-off adapter capability:

preserve_media_message_boundaries = True

Email opts in. Busy-session queueing then uses the existing FIFO instead of album merging. Telegram and every other adapter retain their current default media-burst behavior.

The fix also closes sibling bypasses in the base-adapter fallback, the direct running-agent PHOTO/sentinel paths, and the recursion-depth reset. The latter had a broader FIFO bug: after five or more queued events it could merge/reorder media or overwrite text while resetting recursion depth.

Email now also binds its RFC Message-ID onto SessionSource, matching the MessageEvent.

Related Issue

No matching issue or PR was found after searching open and closed results for email attachment merging and busy-session media queues.

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

  • Add a generic adapter capability for complete media-message boundaries.
  • Opt Email into boundary-preserving FIFO behavior.
  • Route fallback/direct busy paths through the same queue contract.
  • Restore recursion-capped pending events at the FIFO head without merging or reversing them.
  • Bind SessionSource.message_id for Email.
  • Add behavioral coverage for Email FIFO identity, base fallback with and without a runner, recursion-depth restoration, source Message-ID, and unchanged default photo-burst merging.

How to Test

HERMES_PYTHON=/path/to/dev-venv/bin/python \
  scripts/run_tests.sh \
  tests/gateway/test_queue_consumption.py \
  tests/gateway/test_email.py \
  tests/gateway/test_busy_session_ack.py -q

ruff check \
  gateway/platforms/base.py \
  gateway/run.py \
  plugins/platforms/email/adapter.py \
  tests/gateway/test_email.py \
  tests/gateway/test_queue_consumption.py

Result: 134 tests passed; Ruff and git diff --check passed on macOS.

The full repository suite was also started through scripts/run_tests.sh, but this minimal local dev environment lacks the optional acp package and produced unrelated existing credential-test failures. The affected gateway suites above are clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit message follows Conventional Commits
  • I searched existing open and closed issues/PRs
  • The PR contains only this fix
  • Full pytest tests/ -q in a complete all-extras environment
  • Tests were added for the bug
  • Tested on macOS

Documentation & Housekeeping

  • Relevant inline capability documentation updated
  • cli-config.yaml.example — N/A; no user configuration key
  • CONTRIBUTING.md / AGENTS.md — N/A; existing generic architecture retained
  • Cross-platform impact considered; no platform-specific APIs added
  • Tool descriptions/schemas — N/A

@HeyBobChan
HeyBobChan marked this pull request as ready for review July 27, 2026 13:22
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins platform/email Email (IMAP/SMTP) adapter needs-decision Awaiting maintainer decision before any implementation 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 labels Jul 27, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: #65522 preserves all busy-session media events in FIFO order. This PR instead makes boundary preservation Email-specific while retaining album behavior elsewhere; the policy difference needs maintainer selection or consolidation, not a duplicate label.

@HeyBobChan

Copy link
Copy Markdown
Author

Thanks — I compared #65522 closely. This PR deliberately scopes FIFO media preservation behind an adapter capability because the sweeper review on #65522 correctly identified Telegram photo-burst/album merging as an established contract.

The tests here cover both sides explicitly:

  • EmailAdapter.preserve_media_message_boundaries = True: separate queued emails retain their own Message-ID/source/media in FIFO order, including pending-sentinel and recursion-depth paths.
  • default/Telegram adapters: the existing three-photo burst still merges into one album turn unchanged.

Current main already restricts ordinary busy-session steering to text-only events in the relevant paths; #65522's attachment-aware explicit /steer behavior is useful but separable from the email provenance bug. I am happy to consolidate further if maintainers prefer a different capability name or want the /steer part included here.

@HeyBobChan
HeyBobChan force-pushed the codex/email-media-fifo branch from eb4bf14 to 85da555 Compare July 27, 2026 15:57
@HeyBobChan

Copy link
Copy Markdown
Author

Rebased cleanly onto current main (846b14ab) at head 85da5553d351c01d131ea810b86be27d702963a5. The affected gateway suites still pass (134 passed), Ruff is clean, and no intervening upstream commit touched the five affected files.

This boundary fix is the remaining blocker for safely enabling email attachment intake in a downstream production Hermes deployment; we will wait for an official release rather than pin a fork. Could a maintainer please make the queued-media policy decision and, if accepted, include it in a 0.19.x patch release?

@HeyBobChan
HeyBobChan force-pushed the codex/email-media-fifo branch from 85da555 to a478106 Compare July 27, 2026 20:01
@HeyBobChan

Copy link
Copy Markdown
Author

Rebased cleanly onto current main (551e1c6d6) and force-pushed the unchanged boundary-preservation patch at a4781068a.

Fresh verification on the rebased head:

  • ruff check gateway/platforms/base.py gateway/run.py plugins/platforms/email/adapter.py tests/gateway/test_email.py tests/gateway/test_queue_consumption.py
  • scripts/run_tests.sh tests/gateway/test_queue_consumption.py tests/gateway/test_email.py tests/gateway/test_busy_session_ack.py -q — 134 passed
  • git diff --check origin/main...HEAD

All passed.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating Email's complete-message semantics without changing the default album behavior. The premise is confirmed on current main: Email images become PHOTO events at plugins/platforms/email/adapter.py:856-864, and busy-session merging at gateway/platforms/base.py:5694-5700 / gateway/platforms/base.py:2460-2485 folds later media into the first event.

Problems

  • The added tests do not invoke the two runner paths changed by this PR: the direct active-agent PHOTO route (gateway/run.py:14021-14026) and pending-agent-sentinel route (gateway/run.py:14069-14077). Helper-only tests would not catch a future bypass of the boundary-preserving queue contract.

Suggested changes

  • Add active-Email-session regressions through GatewayRunner._handle_message for those paths, asserting distinct RFC Message-IDs, SessionSource values, media, and FIFO order.
  • Salvage the queue restoration against the current SessionState queue API (gateway/run.py:7330-7365), introduced after this PR's base.

Automated hermes-sweeper review.

Comment thread gateway/run.py
@@ -11579,9 +11609,7 @@ async def _handle_message(self, event: MessageEvent) -> Optional[str]:

if event.message_type == MessageType.PHOTO:
logger.debug("PRIORITY photo follow-up for session %s — queueing without interrupt", _quick_key)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an end-to-end active-session Email regression through _handle_message here (including the pending-agent sentinel). The added tests cover _queue_or_replace_pending_event and the base fallback, but do not execute this direct priority path.

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/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/email Email (IMAP/SMTP) adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants