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.
This commit is contained in:
Kayos 2026-05-21 09:09:21 -07:00
parent b681953824
commit 7c8e246544
6 changed files with 244 additions and 243 deletions

14
Cargo.lock generated
View file

@ -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",