Sprint 5 Review: Setup Wizard

Sprint Goal Assessment

Goal: Replace all manual VPS setup with a single aimx setup <domain> command that handles everything from preflight checks to DNS verification.

Assessment: The sprint goal is substantially met. The implementation delivers a complete setup wizard flow: preflight checks, OpenSMTPD configuration, DNS guidance with interactive verification, and setup finalization with DKIM keygen, catchall mailbox, and MCP snippet. The aimx preflight standalone command is also implemented. The code is well-structured with SystemOps and NetworkOps traits for full testability.

Acceptance Criteria Checklist

S5.1 -- Preflight Checks

S5.2 -- OpenSMTPD Configuration

S5.3 -- DNS Guidance + Verification

S5.4 -- Setup Finalization

Bugs Found

BUG 1 (Medium): OpenSMTPD restarted before config.yaml exists

In run_setup() (line 700), configure_opensmtpd() is called which restarts OpenSMTPD with the new smtpd.conf pointing MDA delivery to aimx ingest %{rcpt}. However, finalize_setup() which creates config.yaml, the catchall mailbox, and DKIM keys is called AFTER on line 714. If any email arrives in this window, aimx ingest will fail because config.yaml does not yet exist.

Fix: Move finalize_setup() (and the data_dir/config_path/dkim_selector setup) to BEFORE configure_opensmtpd(), so that by the time OpenSMTPD starts accepting mail, the data directory and config are ready.

BUG 2 (Low): run_preflight_command uses process::exit(1) instead of returning an error

run_preflight_command() at line 743 calls std::process::exit(1) directly when checks fail, bypassing normal error propagation and the error-handling in main(). The run_setup function correctly returns Err(...) for the same scenario (line 697). This is inconsistent and could skip cleanup/destructors.

Fix: Return Err(...) instead of calling process::exit(1), letting main() handle the exit code.

Security Observations (Non-blocking)

1. No domain input validation: The domain argument from the CLI is used directly in smtpd.conf generation without validation. A domain containing double quotes or newlines could inject arbitrary OpenSMTPD config directives. Since this is a root-run CLI tool and the domain is self-provided, the attack surface is self-harm only. Still, basic domain format validation (RFC 1035 characters: alphanumeric, hyphens, dots) would be good defensive coding. Not a blocker.

2. Backup overwrite on repeated setup: Running aimx setup twice overwrites /etc/smtpd.conf.bak with the first run's generated config, losing the original pre-aimx config. Acceptable for now but worth documenting.

Test Coverage

Strong coverage areas:

• 44 unit tests covering all public functions
• Mock-based testing via SystemOps and NetworkOps traits -- excellent design
• Idempotency tests for finalization
• 3 new integration tests for CLI argument parsing
• All 188 unit tests + 32 integration tests pass
• cargo clippy -- -D warnings clean
• cargo fmt -- --check clean

Gaps (non-blocking):

• No test for the full run_setup flow (would require both mock traits and stdin simulation)
• TLS cert generation test only verifies the mock returns Ok, not that the openssl command produces valid output (acceptable since it's behind a trait)
• No test for run_preflight_command specifically

Alignment with PRD

The implementation aligns well with PRD requirements:

• FR-1 (preflight checks): Implemented -- outbound port 25 via gmail-smtp-in, inbound via check.aimx.email
• FR-2 (port 25 blocked error): Implemented -- lists compatible providers
• FR-3 (PTR warning): Implemented -- non-blocking warning
• FR-4 (OpenSMTPD config): Implemented -- TLS, MDA delivery, outbound relay
• FR-5 (DKIM keygen): Integrated into setup finalization
• FR-6 (DNS records display): Implemented -- all 6 record types with actual values
• FR-7 (DNS verification): Implemented -- per-record verification
• FR-9 (catchall mailbox): Implemented
• FR-10 (MCP config snippet): Implemented

The aimx preflight standalone command (FR-42) is also implemented.

Summary

Verdict: CHANGES REQUESTED (posted as comment due to self-owned PR)

The implementation is solid and well-tested. The trait-based abstraction for system and network operations is excellent engineering. Two bugs were found:

1. OpenSMTPD restart ordering (medium) -- OpenSMTPD starts accepting mail before config.yaml exists. Easy fix: reorder the calls in run_setup.
2. process::exit in preflight command (low) -- inconsistent error handling. Easy fix: return Err instead.

Both are straightforward fixes that should not require significant rework. Fix the ordering bug and this is ready to merge.

Re-Review: Sprint 5 Setup Wizard (Round 2)

Previously Flagged Issues

1. Setup ordering (Blocker) -- RESOLVED

Confirmed: finalize_setup() is now called before configure_opensmtpd() in run_setup() (lines 740-742). Config, data dir, catchall mailbox, and DKIM keys are all in place before OpenSMTPD restarts.

2. process::exit(1) replaced with return Err(...) -- RESOLVED

Confirmed: No process::exit calls remain in setup.rs. run_preflight_command() returns Err(...) on failure. Tests cover both success and failure paths.

3. Domain input validation -- RESOLVED

Confirmed: validate_domain() checks empty, max length (253), label length (63), leading/trailing hyphens, allowed characters (alphanumeric + hyphen), and minimum 2 labels. Called at the top of run_setup() before any side effects. Five test cases cover the key cases.

4. Backup overwrite on repeated setup -- ACCEPTED (deferred)

Agree with the reasoning. The backup's purpose is preserving the pre-aimx config, and repeat-setup users are in managed territory. Not worth the complexity now.

New Issue Found

Bug: Catchall address not updated on domain change (Blocker)

In finalize_setup(), when the domain changes (lines 627-629), config.domain is updated and saved, but existing mailbox addresses are NOT updated. Specifically, if catchall already exists with address: "*@old.example.com" and setup is re-run with new.example.com, the config ends up with:

domain: "new.example.com"
mailboxes:
  catchall:
    address: "*@old.example.com"   # stale!

The address field is used for matching incoming mail and as the sender address (see mcp.rs:248, mailbox.rs:51), so this creates a real inconsistency.

Fix: When updating the domain on lines 627-629, also iterate through mailboxes and update any addresses that end with @{old_domain} to use the new domain. The existing test finalize_updates_domain_if_changed should also assert that catchall.address contains the new domain.

This is an edge case (re-running setup with a different domain) but the code explicitly handles the domain-change path, so the address should be updated too.

Verdict: CHANGES REQUESTED

The three previously flagged issues are all properly resolved, and issue 4 is reasonably deferred. However, there is one new bug in the domain-change path of finalize_setup(). Requesting one more fix round.

Re-review (Round 3) -- APPROVED

All previously flagged issues have been verified as resolved:

Round 1 blockers (verified resolved)

1. Setup ordering -- finalize_setup() is now called before configure_opensmtpd() at lines 750-752 of setup.rs. Config, data directory, catchall mailbox, and DKIM keys are all in place before OpenSMTPD restarts.

Round 1 non-blockers (verified resolved)

2. process::exit replaced with Err -- run_preflight_command returns Err(...) on failure, propagated through main(). Tests cover both paths.
3. Domain validation -- validate_domain() enforces RFC 1035 label rules and is called at the top of run_setup() before any side effects. 5 test cases cover valid domains, empty input, single labels, special characters, and hyphen edge cases.

Round 1 non-blockers (deferred, accepted)

4. Backup overwrite on repeated setup -- Accepted as-is per prior discussion.

Round 2 blockers (verified resolved)

5. Mailbox addresses not updated on domain change -- Fixed in commit 4896db7b. When the domain changes in finalize_setup(), all mailbox addresses whose domain portion matches the old domain are rewritten to the new domain (lines 630-638). The finalize_updates_domain_if_changed test now explicitly asserts that catchall.address reflects *@new.example.com.

Verification

• 195 unit tests pass (including domain-change mailbox address assertion)
• 32 integration tests pass
• cargo clippy -- -D warnings clean
• No new issues found

Recommended merge commit message

feat: setup wizard with preflight checks, OpenSMTPD config, and DNS guidance

Add `aimx setup <domain>` command that runs port 25 reachability and PTR
preflight checks, generates config.yaml with a catchall mailbox, creates
DKIM keys, writes and restarts OpenSMTPD with TLS, and displays required
DNS records (A, MX, SPF, DKIM, DMARC, PTR) with inline verification.

Add `aimx preflight` as a standalone check. Domain input is validated
against RFC 1035 label rules. Re-running setup with a different domain
correctly updates all mailbox addresses. Includes MCP config snippet
and Gmail whitelist instructions in post-setup output.