Review mode

This PR is a follow-up behavioral/policy change, not a sprint implementation. No [Sprint N] prefix, and the only sprint that would be in scope (Sprint 26) is already merged and marked DONE. The PRD is modified in this PR (FR-7, FR-19, decision #8) to document the new policy, so I'm reviewing in Standalone Mode against the PR's own stated intent plus consistency with the (now-updated) PRD.

Changes overview

• Adds a new enable_ipv6: bool field to Config (default false, serde #[serde(default)], not exposed by aimx setup).
• LettreTransport becomes stateful (LettreTransport::new(enable_ipv6)); when the flag is false, try_deliver resolves the MX host's A record via ToSocketAddrs and hands the raw IPv4 string to lettre::SmtpTransport::builder_dangerous as the connect target (falling back to the hostname if no A record is found). When the flag is true, the hostname is passed through and the OS chooses the family.
• aimx setup reads enable_ipv6 from an existing config.toml (if present) and skips get_server_ipv6() entirely when the flag is false — AAAA and ip6: SPF are not shown, advertised, or verified.
• PRD FR-7, FR-19, and resolved-decision #8 rewritten to describe the opt-in default.
• book/configuration.md gets a new "IPv6 delivery (advanced)" section; book/setup.md gets a small note.
• Two unit tests added in config.rs covering serde defaulting and parsing.

Scope alignment

Implementation matches the PR description: IPv4-only default, hidden flag, aimx setup gated behind the flag, PRD/book updated. No scope creep observed.

One small scope gap worth calling out: the PR body (and the book) describe aimx verify as "ignoring IPv6 entirely" when the flag is off. The code change gates IPv6 detection inside run_setup, which drives verification through verify_all_dns, so in practice that claim holds for the setup wizard. But aimx verify is its own subcommand — if it has its own IPv6-aware path (not touched in this diff), the claim in book/configuration.md ("aimx setup and aimx verify ignore IPv6 entirely") could be inaccurate. I did not see a verify.rs change in the diff; the author should confirm aimx verify doesn't independently probe IPv6 when the flag is off, or adjust the book wording.

Potential bugs

1. resolve_ipv4() uses a blocking, default-resolver lookup with no timeout — Non-blocker (but worth noting)

src/send.rs:27-36

format!("{host}:25").to_socket_addrs().ok()?.find_map(...)

ToSocketAddrs is a blocking libc getaddrinfo call with no timeout. try_deliver is called from inside block_in_place on a tokio runtime, so a misbehaving/slow DNS resolver will block a worker thread until libc gives up (can be ~30 seconds). The existing resolve_mx uses hickory-resolver with proper async; this new codepath silently drops to libc. Not a correctness issue — just an inconsistency that's easy to overlook. If it matters, reuse hickory-resolver here too.

2. resolve_ipv4() picks the first A record non-deterministically — Non-blocker

ToSocketAddrs iterator order is OS-defined. If an MX host publishes multiple A records (load balancing), you'll connect to whichever one the OS returns first and never retry the others on failure. The outer try_deliver loop only retries across MX hosts, not across A records of a single host. Pre-Sprint-26, lettre itself was doing this resolution and arguably had the same behavior, so this is not a regression — but it's worth a comment in the code so the next reader understands the choice.

3. Error message when A-record lookup fails is silently masked — Non-blocker

If resolve_ipv4(host) returns None (host has AAAA only, or DNS failed), the code falls back to host.to_string() as the connect target. With enable_ipv6 = false, the user's intent is "IPv4 only", but this fallback hands the hostname to lettre, which will then pick whatever family the OS returns — possibly IPv6 — directly contradicting the flag. The user will see a normal "delivered" success and not realize IPv6 was used. Two reasonable alternatives:

• Treat None as an error: "no A record for {host}, skipping (enable_ipv6 = false)" and move on to the next MX.
• Log a visible warning when falling back.

Silently ignoring the flag under a specific edge condition is surprising. At minimum, this deserves a code comment so the behavior is intentional and documented.

4. TLS SNI vs. connect target mismatch is fine here, but worth noting — Non-blocker

TlsParameters::builder(host.to_string()) uses the hostname for SNI / cert verification, while the transport is built with the bare IPv4 string as the connect target. Because dangerous_accept_invalid_certs(true) is set, cert-name mismatch is accepted. Correct, but fragile: if anyone ever flips that flag to do real cert verification, SNI will present host while the TLS layer sees an IP and the behavior gets subtle. Add a comment.

Test coverage

What's covered

• config::tests::enable_ipv6_defaults_to_false and parse_enable_ipv6_true — serde round-trip for the new flag. Fine.
• Every test fixture updated to set enable_ipv6: false. Good hygiene.

Gaps

• No test for LettreTransport::new(true) vs new(false) picking different connect targets. The core behavioral change of the PR has zero direct test coverage. resolve_ipv4 is a private helper, and try_deliver needs a live SMTP receiver — but you could at least unit-test resolve_ipv4("127.0.0.1") returns Some(127.0.0.1) and resolve_ipv4("::1") returns None, or refactor the connect-target selection into a pure function that can be tested without network. As-is, the only verification is the manual strace the author ran on his VPS.
• No test for the aimx setup gating. run_setup now has two branches for enable_ipv6 (true/false). There's no test that, given a config.toml with enable_ipv6 = true, get_server_ipv6() is called, and that with enable_ipv6 = false it is not. The trait-based MockNetworkOps pattern exists specifically for this; the PR did not add an assertion here.
• No test that DNS guidance / SPF generation omits AAAA and ip6: when the flag is false. generate_dns_records is called with server_ipv6_str.as_deref(), so when the flag is false the argument is None. A small test confirming generate_dns_records(..., None, ...) produces no AAAA and no ip6: would lock in the contract.

These aren't theoretical — the PR's whole raison d'être is changing which family is used, and none of that change is under test.

Security issues

None identified. The change is a narrowing of behavior (default to IPv4 instead of OS-choice) and an opt-in flag. No new attack surface.

Code quality

Non-blocker observations

• resolve_ipv4 is defined in impl LettreTransport (line 22) but is an associated function with no self. It could live anywhere; fine as-is, but noting it isn't really a method.
• The PR description mentions "re-introduces resolve_ipv4()" — Sprint 25 removed a similar helper and Sprint 26 cleaned it up. The codebase has now had this helper added, removed, and re-added in three consecutive sprints. Worth a code comment explaining why it's back (to preserve the flag semantic, not because lettre can't connect) so the next engineer doesn't delete it again.
• enable_ipv6: bool field on Config is #[serde(default)] but does not have skip_serializing_if. When save() writes the config, it will always emit enable_ipv6 = false for existing configs that didn't have it. That's probably fine (self-documenting), but inconsistent with verify_host which uses skip_serializing_if = "Option::is_none". Decide which pattern you want and apply uniformly.

Alignment with PRD

PRD FR-7, FR-19, and decision #8 are updated in this same PR to match the new behavior. The updates read cleanly and are self-consistent. No PRD-vs-sprint divergence, because no sprint in docs/sprint.md describes this work — it's a post-sprint policy change.

One minor: docs/sprint.md still has Sprint 26 marked [DONE] with acceptance criteria that say the OS should choose IPv4 or IPv6 naturally. That's now technically inaccurate for the shipped behavior (IPv4 by default). Not a blocker for this PR, but the sprint file would benefit from a note — either via the scrum-plan-updater or a one-line addendum to Sprint 26 — that Sprint 26's default was flipped by this follow-up. Otherwise someone reading sprint.md will think IPv6-by-default is still the behavior.

Human review comments

No human comments on PR #50 or referenced issues. Skipping.

Summary and recommended actions

Overall verdict: Needs minor fixes

The behavioral change itself is sound, small, and well-motivated. The implementation has no correctness blockers I can identify, but the test coverage gaps and the silent resolve_ipv4 → None fallback are worth addressing before merge, because the whole point of the PR is that the default family selection is reliable and predictable.

Blockers (must fix before merge): none.

Non-blockers (should fix):

1. Add direct test coverage for the IPv4-vs-hostname connect-target choice in LettreTransport (at minimum refactor the selection into a pure testable function; ideally a small unit test).
2. Add a test that run_setup calls get_server_ipv6() iff enable_ipv6 is true (use the existing MockNetworkOps).
3. Decide what happens when enable_ipv6 = false and the MX host has no A record. Current behavior silently falls through to the hostname and may use IPv6 anyway, contradicting the flag. Either skip that MX, warn, or at minimum document the fallback in a code comment.
4. Verify and, if needed, correct the book claim that "aimx verify ignores IPv6 entirely" — the diff only gates run_setup; confirm aimx verify as a standalone command behaves the same.
5. Update docs/sprint.md so Sprint 26's acceptance criteria / description note that IPv6 became opt-in via this follow-up. Prevents future confusion.

Nice-to-haves:

• Replace ToSocketAddrs with hickory-resolver in resolve_ipv4 for consistency and timeout control.
• Add a one-line code comment in resolve_ipv4 explaining why it exists (flag semantics, not a lettre bug) to prevent a fourth round of add/remove.
• Decide a single convention for optional vs. defaulted config fields (skip_serializing_if vs. bare #[serde(default)]) and apply consistently.

Re-review (standalone mode) — follow-up on commit 85ec2ed

Verified the five previously flagged non-blockers and all nice-to-haves are addressed. cargo test (365 unit + 44 integration), cargo clippy -- -D warnings, and cargo fmt -- --check all pass on the PR head.

Resolved issues

1. Connect-target selection test coverage (non-blocker #1) — Resolved. select_connect_target(host, enable_ipv6, &ipv4_addrs) -> ConnectTarget extracted as a pure function with a ConnectTarget::{Target, SkipNoIpv4} enum. Four unit tests in src/send.rs cover: IPv6-on + no addrs, IPv6-on + ignored addrs, IPv4-mode + first A record, IPv4-mode + no A record. All paths of the decision are now under test.

2. run_setup gating test (non-blocker #2) — Resolved. detect_server_ipv6(enable_ipv6, net) extracted from run_setup and MockNetworkOps grew a get_server_ipv6_calls: Cell<u32> counter. Three tests assert the exact contract: flag=false → 0 calls + None; flag=true → exactly 1 call; flag=true + underlying None → 1 call + None. This is exactly the test shape that was missing.

3. Silent fallback in resolve_ipv4 (non-blocker #3) — Resolved. ConnectTarget::SkipNoIpv4 is returned when enable_ipv6 = false and the MX host has no A record; try_deliver converts that into an error ("<host>: no A record (enable_ipv6 = false); skipping") so the outer MX loop moves to the next MX instead of silently handing the hostname to lettre. Flag contract is no longer silently violated.

   Small residual note (not a blocker): Self::resolve_ipv4(host).unwrap_or_default() at src/send.rs:177 conflates transient DNS errors (SERVFAIL, timeout, resolver init failure) with "no A record" — both produce an empty vec and the same SkipNoIpv4 message. Because the outer MX loop handles failover, the flag semantic is preserved, but a user troubleshooting DNS issues will see "no A record" even when the real problem is a resolver/network error. Worth a follow-up, not blocking.

4. Doc/behavior mismatch for aimx verify (non-blocker #4) — Resolved. Inspected src/verify.rs directly: it only probes port 25 connectivity (binds a temp listener if free, then run_with_net). No IPv6-aware path. The book copy is now accurate: "aimx verify only probes port 25 connectivity and is unaffected by this flag."

5. Sprint 26 historical record (non-blocker #5) — Resolved. A post-merge addendum block under Sprint 26 in docs/sprint.md clearly states the default was flipped by this PR and points to FR-7, FR-19, decision #8, and the book. Original ACs preserved so the sprint record stays historically accurate.

Nice-to-haves addressed

• hickory-resolver in place of ToSocketAddrs: mx::resolve_a uses ipv4_lookup, treats NoRecordsFound / NxDomain as empty vec, and surfaces other errors. Called from resolve_ipv4 via block_in_place — the same pattern as resolve_mx — so no blocking getaddrinfo in tokio workers.
• Doc comment on resolve_ipv4: added, explicitly documenting the three-sprint add/remove history and the flag-semantics reason it must stay.
• TLS SNI vs connect-target code comment: added in try_deliver, documenting that this is safe only while dangerous_accept_invalid_certs(true) is set.

Intentionally left as-is — acknowledged

• Serde convention unification: author's reasoning is sound — bool and Option<String> have genuinely different "missing" semantics, and adding skip_serializing_if + a fn is_false helper for a bool default would add noise for no operator benefit. Accepted.
• Multi-A-record retry on connect failure: separable concern; the outer MX loop provides sufficient failover for the flag's intent. Accepted.

New issues found

None material. Just the residual unwrap_or_default() note above, which is non-blocking.

Summary

All five previously flagged non-blockers and the three nice-to-haves are resolved with appropriate test coverage. The behavioral change (IPv4-only default, flag honoured even in AAAA-only edge cases) is now properly under test via the pure-function extractions in both send.rs and setup.rs. Documentation is consistent with the implementation. No regressions observed.

Overall verdict: Ready to merge

Recommended merge commit message

Make IPv6 outbound opt-in via enable_ipv6 config flag

Defaults `aimx send` (and `aimx setup`'s DNS guidance/verification)
back to IPv4-only, matching the SPF record `aimx setup` generates by
default. IPv6 outbound and dual-stack DNS guidance become opt-in via
a new hidden `enable_ipv6: bool` field in `config.toml`.

- Config: add `enable_ipv6` (default false) with serde round-trip tests
- send.rs: `LettreTransport` carries the flag; extract pure
  `select_connect_target()` + `ConnectTarget` enum; skip MX hosts with
  no A record when flag is off (via `SkipNoIpv4`) so the flag is not
  silently violated; resolve A records via `hickory-resolver`
- setup.rs: extract `detect_server_ipv6(enable_ipv6, net)` so
  `get_server_ipv6()` is only called when the flag is true
- mx.rs: new `resolve_a()` helper (ipv4_lookup, NoRecords → empty vec)
- Book: add "IPv6 delivery (advanced)" section; correct `aimx verify`
  wording (port 25 only, unaffected by flag)
- PRD: update FR-7, FR-19, resolved-decision #8 to describe opt-in
- Sprint plan: post-merge addendum under Sprint 26 noting the flip

Verified live: default config connects over IPv4 (AF_INET); adding
`enable_ipv6 = true` makes it connect over IPv6 (AF_INET6).