[Sprint 5.5] Non-blocking Cleanup by uzyn · Pull Request #7 · uzyn/aimx

uzyn · 2026-04-09T19:34:47Z

Summary

S5.5-1: Replace unwrap_or_default() with expect() on YAML serialization; narrow tokio features from "full" to rt-multi-thread, macros, io-util, io-std
S5.5-2: Add unit test for write_common_headers covering references = Some(...) path
S5.5-3: Deduplicate DNS resolver creation across DKIM/SPF verification; fix SPF domain fallback to prefer sender's From domain over recipient domain; add Gmail DKIM-signed .eml fixture; verify mail-auth dkim_headers is stable public API via compile-time test
S5.5-4: Implement timestamped backup (smtpd.conf.bak.YYYYMMDDHHmmSS) for pre-aimx OpenSMTPD config to avoid overwriting on repeated setup runs

Test plan

Address accumulated non-blocking review feedback from Sprints 2.5-5: - Replace unwrap_or_default() with expect() on serde_yaml serialization - Narrow tokio features from "full" to rt-multi-thread, macros, io-util, io-std - Add unit test for write_common_headers with explicit references - Deduplicate DNS resolver creation across DKIM/SPF verification - Fix SPF domain fallback to use sender's From domain before rcpt domain - Add Gmail DKIM-signed .eml fixture for parser testing - Verify mail-auth dkim_headers field is stable public API via compile-time test - Implement timestamped backup for pre-aimx OpenSMTPD config

uzyn

Sprint 5.5 Review: Non-blocking Cleanup

Sprint Goal Assessment

Goal: Address accumulated non-blocking improvements from sprint reviews (Sprints 2.5-5).

Result: All 8 items across 4 work areas have been completed. The changes are focused, well-scoped, and address the exact issues flagged in prior reviews. No scope creep.

Acceptance Criteria Checklist

S5.5-1: Serialization + Error Handling

MET — Replace unwrap_or_default() on serde_yaml::to_string() with expect(): Changed in format_markdown() at ingest.rs:422. The expect() message is descriptive.
MET — Narrow tokio features from "full" to specific needed features: Changed to rt-multi-thread, macros, io-util, io-std in Cargo.toml. Cargo.lock correctly drops unused dependencies (signal-hook-registry, parking_lot).

S5.5-2: Send Module Improvements

MET — Add unit test for write_common_headers with references = Some(...) path: New test references_provided_uses_explicit_value in send.rs verifies that explicit References header is emitted as-is rather than being replaced by the reply_to value.

S5.5-3: Channel/Ingest Improvements

MET — Deduplicate DNS resolver creation: New create_resolver() function extracted; resolver is created once in verify_auth() and passed by reference to both verify_dkim_async and verify_spf_async. Clean factoring.
MET — Fix SPF domain fallback semantics: The old code used rcpt (recipient domain) as the SPF fallback domain, which is semantically wrong — SPF checks the sender's domain. The new code correctly prefers the From header's domain and only falls back to the recipient domain when no From is present. Variable naming (from_domain, helo_domain) is now clear.
MET — Add captured DKIM-signed .eml fixture from Gmail: tests/fixtures/gmail_dkim_signed.eml added with realistic Gmail headers including DKIM-Signature, X-Google-DKIM-Signature, and Received headers. Fixture uses fake signature bodies (clearly marked) which is appropriate for parser testing.
MET — Verify mail-auth dkim_headers field is stable public API: Test gmail_dkim_fixture_has_dkim_headers accesses auth_msg.dkim_headers directly — this is a compile-time stability check that will break the build if the field becomes private or is renamed.

S5.5-4: Setup Improvements

MET — Implement timestamped backup for pre-aimx OpenSMTPD config: Uses chrono::Utc::now().format("%Y%m%d%H%M%S") to generate /etc/smtpd.conf.bak.YYYYMMDDHHmmSS. Test verifies 14-digit all-numeric timestamp format.

Test Coverage

201 unit tests pass, 32 integration tests pass — confirmed locally.
clippy and fmt clean.
7 new tests added, covering every acceptance criterion:
- references_provided_uses_explicit_value (send.rs)
- gmail_dkim_fixture_parses_correctly (ingest.rs)
- gmail_dkim_fixture_has_dkim_headers (ingest.rs)
- gmail_dkim_fixture_extracts_received_ip (ingest.rs)
- spf_fallback_uses_from_domain_first (ingest.rs)
- spf_fallback_uses_rcpt_domain_when_no_from (ingest.rs)
- opensmtpd_backs_up_existing_config_with_timestamp (setup.rs)

Test Coverage Observation (non-blocking)

The two SPF fallback tests (spf_fallback_uses_from_domain_first and spf_fallback_uses_rcpt_domain_when_no_from) duplicate the domain-selection logic inline rather than calling verify_spf_async directly. This means they validate the concept of the fallback logic but do not exercise the actual production code path. This is understandable since verify_spf_async requires a real DNS resolver, but worth noting: if someone refactors the domain-selection logic in verify_spf_async without updating these tests, the tests would still pass even if the production code regressed. A future improvement could extract the domain-selection logic into its own testable function.

Potential Bugs

None found. The changes are mechanical and well-tested.

Security Issues

None. The expect() change could theoretically panic at runtime, but serde_yaml::to_string() on a plain struct with #[derive(Serialize)] should never fail unless the serializer itself is broken — expect() is the correct choice here per the sprint requirement.

Alignment with PRD

All changes are maintenance/cleanup of features already implemented in earlier sprints. They improve code quality and correctness (especially the SPF domain fallback fix) without changing external behavior. Fully aligned with PRD goals.

Summary

Verdict: APPROVED (posted as comment due to GitHub self-review restriction)

Clean cleanup sprint. All 8 items completed. The resolver deduplication and SPF domain fix are the highest-value changes — the former eliminates redundant DNS resolver creation per email, and the latter fixes a real semantic bug where SPF was being checked against the recipient's domain rather than the sender's domain. No bugs, no security issues, code quality is good.

Recommended merge commit message:

Non-blocking cleanup: fix SPF domain fallback, dedup resolver, narrow tokio features, timestamped backups

- Replace serde_yaml unwrap_or_default with expect for clearer panics
- Narrow tokio features from "full" to rt-multi-thread, macros, io-util, io-std
- Add unit test for write_common_headers References path
- Extract shared create_resolver() for DKIM/SPF verification
- Fix SPF to check sender's From domain before falling back to recipient domain
- Add Gmail DKIM-signed .eml fixture for parser testing
- Add compile-time check that mail-auth dkim_headers is public API
- Use timestamped backups (smtpd.conf.bak.YYYYMMDDHHmmSS) for OpenSMTPD config

…lans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md

* docs: add manual test results with findings Full execution of docs/manual-test.md against agent.zeroshot.lol. 10 findings recorded with severity and fix direction, notably: - P0 DKIM key on disk does not match DNS TXT (root cause of outbound dkim=fail at Gmail) - P0 Shell injection in on_receive cmd template expansion - P1 MCP write ops (email_mark_read, etc.) fail when MCP runs as non-root due to root:root 0644 mailbox files * docs: add Recommended fixes section with per-finding implementation plans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md

Sprint 44 (post-launch security + quick fixes) addresses findings #9, #10, #7, #1-tier-1, #3. Sprint 45 (strict outbound + MCP writes via daemon addresses #4, #5, #8 and introduces UDS MARK-READ/MARK-UNREAD verbs. Sprint 46 (mailbox CRUD via UDS) addresses #1-tier-2 with MAILBOX-CREATE/MAILBOX-DELETE verbs so daemon picks up changes live. PRD FR-18d tightened: outbound send now requires a concrete non-wildcard mailbox under config.domain; catchall is inbound-only. PRD FR-18e added to cover the new state-mutation verbs on the UDS socket. Finding #2 (SPF envelope MAIL FROM) excluded — already shipped in cd22428. EOF )

Closes the highest-priority findings from the 2026-04-17 manual test run with small, targeted patches. S44-1 — Env-var channel-trigger expansion (fix shell injection, P0) Drop `{from}`/`{subject}`/`{to}`/`{mailbox}`/`{filepath}` substitution in `on_receive.command`. User-controlled header values now ride on `AIMX_FROM` / `AIMX_SUBJECT` / `AIMX_TO` / `AIMX_MAILBOX` / `AIMX_FILEPATH` env vars, which the shell expands safely even for attacker-controlled payloads (`$()`, backticks, `;`, quotes, newlines). Only aimx-generated `{id}` / `{date}` stay as string substitutions. Legacy placeholders are rejected at config-load time with a migration hint naming the offending mailbox. - Remove `shell_escape` crate dependency. - Rewrite book/channel-recipes.md, book/channels.md, docs/manual-test.md T8 to use `"$AIMX_*"` inside double quotes. - New unit tests: angle-bracket From (`U-Zyn Chua <chua@uzyn.com>`), `` `whoami` ``, `$(rm -rf /)`, `foo; ls`, newline, mixed quotes — all land in env vars verbatim, none execute. - `Config::load` calls `validate_on_receive_commands` and refuses. S44-2 — DKIM DNS sanity check at daemon startup + louder setup warning (P0) - `serve.rs`: after DKIM key load, resolve `<selector>._domainkey.<domain>` via hickory, compare DNS `p=` to on-disk SPKI base64. Never fatal — DNS may not have propagated yet. `DkimTxtResolver` trait + pure `evaluate_dkim_startup` enum for unit-testable outcomes (Match / Mismatch / NoRecord / NoPTag / ResolveError). Production resolver runs a one-shot hickory TXT lookup via `block_in_place`. - `setup.rs` `display_dns_verification`: refactored into `dns_verification_lines` so tests can assert output. DKIM FAIL / MISSING branches now print a second, semantically-red line: "Outbound DKIM signatures will FAIL verification at receivers until DNS matches." - New `dkim::public_key_spki_base64` + `dkim::extract_dkim_p_value` helpers; `setup::extract_dkim_public_key` now delegates to them so the setup-time check and the startup check share one parser. S44-3 — `aimx agent-setup claude-code` hint fix (P1) Rewrite `claude_code_hint` to mirror Codex's structure (install location line, blank, `claude mcp add --scope user aimx /usr/local/bin/aimx mcp` command, blank, restart note). Threads `--data-dir` into the command when an override is set. Claude Code does NOT auto-activate MCP servers from installed plugins — particularly `claude -p` — so the old "auto-discovered" hint led operators down a dead-end path (finding #7). - Update book/agent-integration.md, agents/claude-code/README.md. S44-4 — `aimx mailbox create` / `delete` prints service-restart hint (P2) Follow-up line after the success banner for both `create` and `delete` naming `sudo systemctl restart aimx` (systemd default) or `sudo rc-service aimx restart` (OpenRC) based on `serve::service::detect_init_system()`. Unknown init systems fall back to the systemd wording. Tier-2 (route CRUD via UDS so the daemon picks up live) is Sprint 46. - Update book/mailboxes.md to document the restart requirement. S44-5 — docs/manual-test.md: specify "compose new" for email steps (P3) T3 / T5 / T8 / T9 handoffs now explicitly say "compose a new email" with a note against forwarding/replying to prior threads — the forwarded-message noise on the 2026-04-17 run contaminated frontmatter assertions on `subject` and `in_reply_to` / `references`. All tests pass (`cargo test`: 736 tests, 0 failed across aimx + aimx-verifier). `cargo clippy --all-targets -- -D warnings` clean. `cargo fmt -- --check` clean.

Non-blocking cleanup: fix SPF domain fallback, dedup resolver, narrow tokio features, timestamped backups - Replace serde_yaml unwrap_or_default with expect for clearer panics - Narrow tokio features from "full" to rt-multi-thread, macros, io-util, io-std - Add unit test for write_common_headers References path - Extract shared create_resolver() for DKIM/SPF verification - Fix SPF to check sender's From domain before falling back to recipient domain - Add Gmail DKIM-signed .eml fixture for parser testing - Add compile-time check that mail-auth dkim_headers is public API - Use timestamped backups (smtpd.conf.bak.YYYYMMDDHHmmSS) for OpenSMTPD config

…lans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md

* docs: add manual test results with findings Full execution of docs/manual-test.md against agent.zeroshot.lol. 10 findings recorded with severity and fix direction, notably: - P0 DKIM key on disk does not match DNS TXT (root cause of outbound dkim=fail at Gmail) - P0 Shell injection in on_receive cmd template expansion - P1 MCP write ops (email_mark_read, etc.) fail when MCP runs as non-root due to root:root 0644 mailbox files * docs: add Recommended fixes section with per-finding implementation plans Code-review-backed fix plans for each of the 10 findings, with file:line refs, effort estimates, and a priority order. No code changes yet — this consolidates the investigation so fixes can be sequenced. - #10 DKIM mismatch: not a code bug; DNS republish + optional startup check - #9 shell injection: pass trigger vars via env, not string substitution - #8 MCP writes: route state mutations through daemon UDS - #7 claude-code hint: print `claude mcp add` command post-install - #4 send config read: move mailbox resolution to daemon side - #2 SPF: plumb envelope MAIL FROM from smtp session through ingest - #5 wildcard send: remove wildcard branch from resolve_from_mailbox - #1 mailbox create: add restart hint (or route via daemon) - #3 plan wording: clarify "compose new" in docs/manual-test.md

Sprint 44 (post-launch security + quick fixes) addresses findings #9, #10, #7, #1-tier-1, #3. Sprint 45 (strict outbound + MCP writes via daemon addresses #4, #5, #8 and introduces UDS MARK-READ/MARK-UNREAD verbs. Sprint 46 (mailbox CRUD via UDS) addresses #1-tier-2 with MAILBOX-CREATE/MAILBOX-DELETE verbs so daemon picks up changes live. PRD FR-18d tightened: outbound send now requires a concrete non-wildcard mailbox under config.domain; catchall is inbound-only. PRD FR-18e added to cover the new state-mutation verbs on the UDS socket. Finding #2 (SPF envelope MAIL FROM) excluded — already shipped in f5cebd2. EOF )

uzyn commented Apr 9, 2026

View reviewed changes

uzyn merged commit a6ac68d into main Apr 9, 2026
2 checks passed

uzyn deleted the sprint-5.5-cleanup branch April 10, 2026 00:32

uzyn mentioned this pull request Apr 17, 2026

docs: manual test plan + full test-run results (10 findings) #78

Merged

3 tasks

This was referenced Apr 18, 2026

feat(cli): port-25 preflight first in setup; DNS check in status #88

Merged

feat: guided menu for aimx agent-setup with MCP (General) option #92

Merged

This was referenced Apr 30, 2026

feat: add --port-check-only flag to install.sh #185

Merged

feat(agents): auto-register MCP server for Claude Code & Codex #190

Merged

[Sprint 1] Authz model + UDS handler — owner-bound mailbox CRUD #193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sprint 5.5] Non-blocking Cleanup#7

[Sprint 5.5] Non-blocking Cleanup#7
uzyn merged 1 commit into
mainfrom
sprint-5.5-cleanup

uzyn commented Apr 9, 2026

Uh oh!

uzyn left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

uzyn commented Apr 9, 2026

Summary

Test plan

Uh oh!

uzyn left a comment

Choose a reason for hiding this comment

Sprint 5.5 Review: Non-blocking Cleanup

Sprint Goal Assessment

Acceptance Criteria Checklist

S5.5-1: Serialization + Error Handling

S5.5-2: Send Module Improvements

S5.5-3: Channel/Ingest Improvements

S5.5-4: Setup Improvements

Test Coverage

Test Coverage Observation (non-blocking)

Potential Bugs

Security Issues

Alignment with PRD

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant