feat: add forward_email tool

[jbkjr]

Summary

Adds a forward_email tool — forward an existing message to new recipients, re-attaching the original's attachments. This is a common action that currently isn't possible (you can reply via send_email threading, but not forward with attachments).

Behaviour mirrors send_email: it fetches the original, builds a Fwd: subject (not double-prefixed), appends a plain-text "---------- Forwarded message ----------" block below any optional note, re-attaches the original attachments, sends, and saves a copy to Sent when configured.

Supporting changes (small, and reused by the existing send path)

• EmailClient.extract_attachments — fetch an email and return its attachments as (filename, mime_type, data) tuples, built on the same fetch/parse helpers download_attachment already uses.
• compose_message / _create_message_with_attachments / EmailClient.send_email gain an optional extra_parts argument for attaching prebuilt in-memory MIME parts (the re-forwarded attachments). Existing behaviour is unchanged when it isn't passed.
• Added to the EmailHandler interface; the tool enforces the recipient allowlist exactly like send_email.

Forwarded content is sent as plain text (the parsed body, which is already text even for HTML originals) to stay simple and universally readable; a richer HTML forward could be a follow-up.

Tests

• Handler: subject/body composition, attachment re-attachment, the already-Fwd: case, and the not-found error.
• Tool: dispatch + summary string + allowlist enforcement.
• Full suite passes (uv run pytest), ruff check clean.

[codecov]

Codecov Report

❌ Patch coverage is 98.79518% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
mcp_email_server/emails/classic.py	98.6%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[Wh1isper]

Thanks for adding this. A forward tool is a useful and reasonably scoped addition, and I like that it reuses the existing send path and keeps forwarded content plain text for now.

I found one blocking issue: forward_email currently bypasses the sender allowlist introduced in #180.

In ClassicEmailHandler.forward_email, the original message is fetched with:

original = await self.incoming_client.get_email_body_by_id(email_id, mailbox)

This does not pass allowed_senders=get_settings().allowed_senders. With a sender allowlist configured, a client that knows a blocked sender's UID can still call forward_email and have that blocked message body forwarded out.

The same issue exists for attachments:

await self.incoming_client.extract_attachments(email_id, mailbox)

extract_attachments() currently has no allowed_senders parameter and does not enforce the sender allowlist before fetching the raw message. So a blocked sender's attachments can also be read and forwarded.

Could you update forward_email to pass the configured sender allowlist into both body and attachment reads, and update extract_attachments() to enforce the allowlist before fetching the message body? Blocked messages should remain indistinguishable from missing/inaccessible messages and should not call send_email.

Please also add regression tests for:

• blocked sender cannot be forwarded when allowed_senders is configured
• blocked sender's attachments are not fetched by extract_attachments
• forward_email does not call extract_attachments or send_email when the original is blocked

There is also a MIME preservation issue when re-attaching original attachments:

subtype = mime_type.split("/", 1)[1] if "/" in mime_type else "octet-stream"
part = MIMEApplication(payload, _subtype=subtype)

MIMEApplication always creates an application/* part, so image/png becomes application/png, text/plain becomes application/plain, etc. Please preserve the original main type, for example by using MIMEBase(maintype, subtype) plus base64 encoding, and add a small test for forwarding an image/png attachment.

A few smaller follow-ups:

• The tool description should mention that forwarded content is parsed plain text and may lose HTML formatting / be truncated by the existing body parsing limit.
• The read-only tool visibility tests should include forward_email.
• _format_forwarded_text labels original["to"] as To:, but the existing parser may include Cc recipients in that list. A neutral label like Recipients: may be less misleading.

The rest of the direction looks good: recipient allowlist is enforced at the tool layer, read-only accounts are guarded, and BCC handling follows the existing send/Sent-save pattern.

[jbkjr]

Thanks for the thorough review — all addressed, and rebased onto current main.

Sender allowlist (blocking):

• forward_email now reads get_settings().allowed_senders and threads it into both get_email_body_by_id and extract_attachments. A blocked original returns None (indistinguishable from missing), so it never reaches extract_attachments or send_email.
• extract_attachments() gained an allowed_senders param and checks the From header via _batch_fetch_senders/sender_allowed before fetching the body; a blocked message yields [] (indistinguishable from no-attachments/missing).
• Regression tests added: blocked sender is neither read nor sent; blocked attachments are not fetched; allowed sender is fetched.

MIME preservation: switched from MIMEApplication to MIMEBase(maintype, subtype) + base64, so image/png stays image/png. Added a forward test asserting an image/png attachment keeps its type and payload round-trips.

Smaller follow-ups:

• Tool description now notes forwarded content is parsed plain text (HTML lost; may be truncated by the body limit).
• Read-only tool-visibility tests now assert forward_email (hidden without SMTP, shown with).
• _format_forwarded_text now labels the recipient list Recipients: instead of To:.

Patch coverage is back to ~100%.

[Wh1isper]

wh1isper's agent here: this is an agent-assisted maintainer review of HEAD c535e8cbb79f810ea78a2c8e244c8cc8123c920b.

Thank you for addressing the earlier sender-allowlist and MIME main-type feedback. Those fixes look good. I found three remaining correctness/security blockers before this can merge.

1. Validate email_id before any IMAP I/O

forward_email accepts an unconstrained string and ultimately passes it to imap.uid("fetch", email_id, ...) through _batch_fetch_senders() and _fetch_email_with_formats(). aioimaplib concatenates command arguments and does not reject CR/LF. Inputs such as UID sets or a CRLF-bearing value can therefore change the generated IMAP command and potentially bypass the application mutation controls.

Please add a centralized client/handler validation requiring one decimal IMAP UID in the valid range before opening an IMAP connection. The MCP schema can also use a pattern, but schema validation must not be the only defense. Add tests for CR/LF, spaces, 1:*, 1,2, zero, and out-of-range values, asserting that invalid input performs no IMAP I/O.

The underlying sink is shared by some existing tools, so a centralized fix is preferable to adding another unvalidated path.

2. Do not silently send a forward when attachment retrieval failed

_fetch_email_with_formats() can swallow failures and return None; extract_attachments() then maps missing mail, IMAP failure, raw-message extraction failure, and a genuinely attachment-free message to the same [] result. forward_email() treats that as success and sends a message without the original attachments.

Please distinguish a successfully parsed message with no attachments from an attachment-fetch/parse failure. Any failure must abort before SMTP. Ideally, parse the body and attachments from the same allowlist-protected raw-message fetch to avoid the second IMAP round trip and TOCTOU behavior.

3. Preserve valid .eml and empty attachments

At mcp_email_server/emails/classic.py:1266-1275, get_payload(decode=True) returns None for common message/rfc822 attachments and b"" for zero-byte attachments; the truthiness check drops both. Rebuilding parts from only (filename, mime_type, bytes) also loses MIME parameters and metadata such as smime-type, Content-ID, and Content-Language.

Please preserve/copy the original attachment MIME parts where possible, and add tests for message/rfc822, zero-byte, and nested multipart attachments. A fetch/parse failure must also be covered by a test that asserts send_email is not called.

Please also rebase after #194 lands; the branches currently conflict in tests/test_mcp_tools.py. The full PR suite is green, but it does not exercise these cases.

[Wh1isper]

Thank you for the work on forward_email and for addressing the earlier review feedback.

The feature is still not present on current main, so I’d like to keep this PR open. However, #212 has since replaced the mail architecture, and this branch now conflicts with it. Please rework the implementation against the current application boundaries rather than rebasing the old app.py/classic-handler integration as-is.

The updated implementation should, in particular:

• introduce the forward command/service through the application layer and provider adapter boundary;
• revalidate current account authority and sender/recipient policy immediately before each independent effect;
• use the centralized request, body, attachment, aggregate, provider-work, and serialized-result limits;
• preserve forwarded MIME content and attachments safely without bypassing policy;
• report provider failure/unknown outcomes without unsafe automatic replay;
• update the exact MCP catalog/schema/annotation snapshot and user documentation;
• add focused application/adapter tests plus GreenMail stdio E2E coverage.

Please rework this PR on top of current main, and we’ll review the new design from there. Thanks again for contributing the feature.

[jbkjr]

Reworked from scratch against the V2 application boundaries per your 2026-07-25 review — this replaces the old app.py/classic-handler implementation entirely (single commit off current main, 3a743ee).

Mapping to your rework brief:

• Application layer — ForwardCommand (a ComposeCommand), ForwardSource, and ForwardService in application/mutations.py. The forward runs as three independent provider effects (IMAP source read → SMTP delivery → sent copy), each preceded by a fresh authority/policy resolution via providers.open(...); the recipient allowlist is validated against both the pre-open snapshot and the opened account, mirroring SendService.
• Abort before SMTP — the defect you flagged is fixed structurally, not patched: EmailClient.fetch_forward_source() raises on every failure path (missing, blocked, unreadable, oversized, parse failure — there is no [] sentinel), and those exceptions propagate out of ForwardService.execute before the outgoing provider is ever opened. Tests pin provider.forward.assert_not_awaited() for both the sanitized-provider-failure and sentinel paths. The sender allowlist is enforced before the body fetch, in the same single IMAP session (no second round trip / TOCTOU).
• Centralized limits — part count / per-part / aggregate bytes and derived subject reuse APPLICATION_LIMITS; the derived body (note + forwarded block) is re-validated through ComposeCommand.validate, so an oversized forward is rejected, never truncated.
• MIME preservation — parts are re-attached, not rebuilt from get_payload(decode=True), so message/rfc822 subtrees, zero-byte parts, Content-ID, CTE, and Content-Type parameters survive. Each part is round-tripped through compat32 (normalize_forwarded_part) before attachment: source parts are parsed under policy=default, and attaching a structured part directly loses the RFC 2231 charset label when the header needs refolding — the same input then serializes differently on the SMTP, SMTPUTF8, and IMAP-append paths (filename*=unknown-8bit''… on some). The round-trip makes all paths byte-identical; the regression test uses an unfolded ≥86-char Content-Disposition fixture because a pre-folded one hides the defect.
• Outcome reporting — reuses SendMutationOutcome and the existing detail tags; ambiguous SMTP stays unknown + reconciliation_needed, never replayed. The sent-copy tail of SendService.execute is extracted into a shared _complete_send() used by both services so those semantics live in one place (SendService behavior unchanged, existing tests untouched).
• Catalog/docs/spec — catalog fixture regenerated (forward_email sits directly after send_email, annotated non-read-only/non-idempotent/open-world like the other additions); docs/tools.md/guides.md/configuration.md updated; spec/07 gains a Forward subsection + acceptance criterion 07.12, and spec/12's verification row is extended.
• E2E — GreenMail stdio coverage seeds a message with an attachment, forwards alice→bob, and verifies the delivered message, the re-attached part (content type + filename), and the Sent copy over imaplib rather than trusting the server response; plus an allowlist-denial case asserting nothing is delivered.

Local gates: full suite 1347 passed (+83 new), make test-e2e 5 passed, pyright 0 errors, make check and mkdocs build --strict clean.

Two deliberate calls to flag:

1. smtp-8bitmime-required pre-flight rejection (added to _PUBLIC_SEND_DETAILS): if forwarded parts would put raw 8-bit octets on a session that didn't advertise 8BITMIME, the transaction fails before MAIL, mirroring smtp-utf8-unsupported. A payload-preserving re-encode isn't possible in general — for a binary part with no charset param the stdlib's downgrade path raises UnicodeEncodeError, and RFC 2046 forbids base64-ing multipart/*/message/rfc822. The underlying gap (no general 7-bit downgrade in send_email_with_outcome) is pre-existing and orthogonal; filed separately rather than folded into this PR.
2. spec/12 review-disposition cell — extending the row to 07.12 means the existing "no unresolved material findings" wording now nominally covers a criterion you haven't reviewed. I left that maintainer-owned cell untouched for you to disposition.

[codecov]

Codecov Report

❌ Patch coverage is 95.30516% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.7%. Comparing base (8b3c002) to head (a7ac903).

Files with missing lines	Patch %	Lines
mcp_email_server/application/mutations.py	93.7%	3 Missing and 4 partials ⚠️
mcp_email_server/emails/classic.py	96.8%	2 Missing ⚠️
mcp_email_server/app.py	92.8%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #184     +/-   ##
=======================================
+ Coverage   84.3%   84.7%   +0.4%     
=======================================
  Files         30      30             
  Lines       8545    8726    +181     
  Branches    1185    1210     +25     
=======================================
+ Hits        7204    7395    +191     
+ Misses       942     935      -7     
+ Partials     399     396      -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Wh1isper]

Thank you for reworking this from scratch against the current application/provider boundaries. This is a substantial improvement over the previous implementation: the source read is now allowlist-protected and fail-closed, MIME parts are retained rather than rebuilt from decoded payloads, authority is reopened between effects, and delivery/Sent-copy ambiguity reuses the existing send semantics.

I reviewed HEAD 3a743ee935f83402d1514c70fdfa87144c32f821. I found one P1 content-integrity blocker and one P2 capability-ordering issue before this is ready to merge. The shared SMTP 7-bit/8-bit/binary transport concern will be handled separately in #233, after which this branch should rebase and reuse that common implementation.

P1: the forwarded body is still silently truncated at 20,000 characters

fetch_forward_source() calls:

email_data = self._parse_email_data(raw_email, email_id)

without overriding _parse_email_data()'s MAX_BODY_LENGTH = 20000 default. _parse_email_data() therefore replaces a longer body with the first 20,000 characters plus ...[TRUNCATED], and the application layer validates and sends that already-truncated value.

This contradicts the new documentation, which says that forwarded content is never silently truncated and that an oversized composed body is rejected. It also recreates the silent content-loss behavior the new fail-closed source-read design is intended to prevent.

Please make the forward path parse enough content for APPLICATION_LIMITS.body_bytes to remain authoritative. One simple option is to use a forward-specific window larger than the byte limit, so any parser truncation necessarily produces a value that application validation rejects rather than sends.

Please add concrete-provider regression tests proving that:

• a source body longer than 20,000 characters but within the 1 MiB compose limit is forwarded in full;
• a source body exceeding the compose limit is rejected before the outgoing provider is opened.

P2: an IMAP-only account reads the complete source before discovering that it cannot send

ForwardService.execute() opens the incoming provider and fetches the complete source before it opens the outgoing provider. The absence of SMTP is only detected later in ClassicMutationProvider.forward():

client = self._handler.outgoing_client
if client is None:
    raise MutationProviderError(...)

An IMAP-only account therefore logs in, downloads, and parses the source message and attachments before the operation reports that forwarding is unavailable. This does not match the new configuration documentation, which says the source read occurs only after the send-capability check.

Please reject a non-send-capable account before source-message I/O, while preserving late secret resolution. I would avoid opening the outgoing provider merely as a capability probe, because that resolves the outgoing secret too early; exposing a non-secret send-capability flag in the authority snapshot would keep the boundary cleaner.

Please add an application-level test proving that an IMAP-only account performs no incoming provider access for forward_email.

Shared SMTP transport handling will be addressed in #233

The new smtp-8bitmime-required preflight is directionally correct and is a reasonable fail-closed response to forwarded parts containing raw 8-bit octets.

However, the current extra_parts-specific check is not a complete SMTP transport classifier. In particular, bytes.isascii() does not detect NUL or other content requiring binary transport, and 8BITMIME does not make Content-Transfer-Encoding: binary safe to submit through SMTP DATA.

This is a shared send_email_with_outcome boundary rather than a forward-specific concern, so we will address the general transport classification in #233 instead of asking this PR to grow a second partial implementation. The intended scope for #233 is conservative pre-MAIL validation, not a recursive MIME downgrade.

Please focus this PR on the two forward-specific findings above. Once #233 lands, please rebase this branch and use the shared transport handling rather than retaining the extra_parts-specific _part_emits_raw_8bit() guard.

The remaining direction looks good. In particular, the single-session allowlist-protected source read, fail-closed attachment handling, retained MIME subtrees, centralized limits, and shared delivery/Sent-copy outcome semantics are all aligned with the current architecture.

[jbkjr]

Both findings addressed, and the branch is rebased onto current main (8b3c002, #234) — new head is a single commit replacing the old one.

P1 — silent 20k truncation (fixed): fetch_forward_source() now parses with a forward-specific window, FORWARD_SOURCE_BODY_WINDOW = APPLICATION_LIMITS.body_bytes + 1 characters. Since one character encodes to at least one UTF-8 byte, any parser truncation necessarily leaves a body over the compose byte limit, so application validation (body exceeds … bytes, raised by forwarded.validate() before the outgoing provider is opened) stays authoritative: a long source is forwarded in full or rejected outright, never silently shortened. Concrete-provider regression tests added in TestFetchForwardSource:

• a 30,000-character source body is returned in full with no ...[TRUNCATED] marker;
• a source body of body_bytes + 10 still yields an over-limit value after parsing, pinning the invariant that truncation can never produce a sendable body (the existing application tests already pin that an over-limit derived body aborts before the outgoing open).

Both tests were verified to fail against the previous code before the fix.

P2 — IMAP-only account reads the source before failing (fixed): MutationAccountSnapshot gains a non-secret can_send flag populated from outgoing endpoint presence (EmailSettings.can_send; in managed mode _read_account_authority reads endpoint rows role-independently, so no outgoing secret is resolved). ForwardService.execute() rejects a send-incapable account with the same capability_unavailable error the provider raises today — before the incoming provider is opened, so the source message is never logged into, downloaded, or parsed — and re-checks the flag on the freshly opened incoming authority. Opening the outgoing provider remains the enforcing boundary. Application tests added: an IMAP-only account performs no provider access at all (factory.open never called, no source fetch), and capability loss discovered on the reopened incoming authority aborts before retrieval. docs/configuration.md's "source read is attempted only after that capability check" is now true as written.

Shared transport handling (#233/#234): _part_emits_raw_8bit() and the extra_parts-specific preflight are gone; the forward path now rides _classify_smtp_data_transport unchanged. One integration point to flag: a re-attached source part with a correctly labeled 8bit CTE sits inside the composed multipart/mixed container, and with the container's CTE defaulting to 7bit the classifier correctly refuses the whole message as a mislabeled composite — even with 8BITMIME advertised. compose_message therefore labels the container Content-Transfer-Encoding: 8bit when any re-attached part serializes non-ASCII (RFC 2045 §6.4: a composite entity must declare the domain of its contents). Result: a correctly labeled 8-bit source part is delivered with BODY=8BITMIME when advertised and fails with the shared smtp-8bitmime-required when not, while mislabeled and binary source parts fail with the shared smtp-mime-transport-invalid / smtp-binarymime-unsupported diagnostics. The over-label is inert for 7-bit content (classification still returns 7bit and no BODY parameter is requested). Tests updated to pin this against the shared classifier; docs/tools.md's forward section now defers to the shared transport documentation.

Spec: #234 took acceptance criterion 07.12, so the forward criterion is now 07.13; spec/07's Forward section gains the send-capability precondition and the no-silent-truncation parse-window contract, and the spec/12 row is extended to 07.1-07.13 with disposition left at your "Pending independent review" (which also resolves the maintainer-owned-cell question from my previous comment).

Local gates: make check clean, full suite 1384 passed, pyright 0 errors, mkdocs build --strict clean, make test-e2e 5 passed on GreenMail.

[Wh1isper]

@jbkjr I think we're not far from a complete merge. Please resolve the conflicts and have agent review it again.

[jbkjr]

Conflicts resolved and the agent review re-run, per your request. New head: 431a857.

On the conflicts: my mistake — yesterday's rework was force-pushed to the wrong branch of my fork (feat/forward-email-clean, a sibling that happened to share the old tip), so the PR head never moved and GitHub kept showing the stale 3a743ee as conflicting. There were no real conflicts against current main; the rebased rework is now on the PR's actual head branch and the PR reports mergeable.

Agent review: I ran a ten-angle multi-agent review over the full diff (line-by-line, removed-behavior, cross-file tracing, language pitfalls, wrapper correctness, plus reuse/simplification/efficiency/altitude/conventions), adversarially verified the contested findings by reproduction, and finished with an independent gap sweep. It surfaced real issues, which this amendment fixes:

• Envelope-header leak (worst finding): a single-part source whose top level is the attachment (Content-Disposition: attachment on the root) was re-attached whole — Received chain, DKIM, Message-ID, and the Bcc header a Sent-folder copy carries included. Re-attached roots are now stripped to their Content-* headers, with a regression test.
• Input-contract locks: ForwardCommand.validate now rejects a caller-supplied subject (previously validated then silently overwritten), html=True (would mislabel the plain-text block), and caller attachments (whose budget was counted separately from forwarded parts, allowing 2× the ceiling). The MCP tool never exposed these fields; the locks close the application-layer boundary. The derived command revalidates through the shared ComposeCommand contract.
• Capability symmetry: can_send is now a required snapshot field (no fail-open default), checked by both submission workflows on every snapshot including the outgoing access — in compatibility mode the outgoing open doesn't enforce role presence, so send and forward previously failed differently for the same IMAP-only account.
• Fixed-shape errors and honest provenance: the forward parse-failure message no longer interpolates raw parser exception text (which can embed source bytes); a source with no Date header now omits the quote-block Date line instead of fabricating now().
• One parse instead of two for the source message (up to 50 MiB), and the container's 8bit label is now derived from _classify_mime_entity_transport itself rather than a parallel isascii() rule, so labeling and classification cannot disagree — and no extra serialization.
• Cleanups: shared _submit in the adapter (send/forward had duplicated the eleven-argument SMTP call and had already drifted on reply_to), write-only ForwardSource fields dropped, duplicated validation/result predicates consolidated, and the three tool descriptions plus security/troubleshooting/guides/tools docs that still omitted forward_email from the capability/allowlist enumerations updated (catalog fixture regenerated). The spec sentence claiming all three effects revalidate "capability, and policy" was corrected — the sent copy follows the pre-existing authority-change rules.

Two behaviors surfaced by the review that I deliberately did not change, flagged for your disposition:

1. A forwarded composite part (message/rfc822 capsule or nested multipart) whose 8-bit leaf is correctly labeled but whose container lacks its own 8bit CTE classifies invalid and the forward is refused with smtp-mime-transport-invalid — even with 8BITMIME. That is your classifier's documented mislabeled-composite rule; repairing it would mean relabeling re-attached source containers, i.e. the recursive MIME rewriting send_email_with_outcome has no 7-bit downgrade path for raw 8-bit message content #233/fix(smtp): validate DATA transport requirements #234 scoped out, so I left it fail-closed. Such sources are non-conformant per RFC 2045 §6.4 but do occur in real mail (an attached .eml with an 8-bit body and no capsule CTE). If you'd rather the forward path widen container labels recursively (labels only — payloads untouched), I'm happy to do that as a follow-up.
2. A non-conformant source header parameter stored as raw 8-bit octets (unencoded filename="café.pdf") is re-folded to RFC 2231 unknown-8bit form by the normalize round-trip — standard-conformant and path-consistent, but not byte-identical to the source. Documented as a trade-off in the docstring; preserving the original bytes would need a compat32-first parallel read.

Also noted as small follow-ups rather than grown into this PR: the Sent copy of an 8-bit forward rides a plain IMAP literal (fine for GreenMail/Dovecot-class servers; a literal8/BINARY path would be a separate change), the per-part serialize count on the forward path, and deduplicating the three near-identical IMAP read prologues.

Local gates on 431a857: make check clean, 1390 unit tests passed (+22 from the review fixes), pyright 0 errors, strict docs build, GreenMail E2E 5 passed (now also pinning that the source read never sets \Seen).

[Wh1isper]

Reviewed current head after maintainer fixes. Forwarded-part bounds now use the conservative SMTP/SMTPUTF8 wire serialization size, source sender policy is revalidated before SMTP with privacy-preserving precedence, and the spec/docs/tests match the implementation. Local make check, make test (1392 passed), make docs-test, and GreenMail E2E (5 passed) all pass.