Sprint 2 Review — email_list paginated rewrite

Sprint Goal Assessment

The sprint goal — "Replace the filter-laden email_list with an index-free, paginated, JSON-shaped tool. Listing N rows reads at most N frontmatter blocks regardless of mailbox size" — is achieved. The two-pass algorithm (enumerate ids → sort descending → read frontmatter only for the page slice) is correctly implemented in enumerate_email_ids + list_email_page_json, and the perf claim is pinned by an instrumented test that hard-asserts ≤ limit frontmatter reads on a 10k-message mailbox. The JSON shape (separate inbox vs. sent rows, with read absent on sent) matches FR-B3 exactly. The schema correctly drops unread / from / since / subject and gains limit / offset. Docs were updated across the primer, references, and book/.

Acceptance Criteria Checklist

S2-1: Filename-sort + page-bounded frontmatter algorithm

• EmailListParams loses unread / from / since / subject; gains limit: Option<u32> (default 50, clamped to 200) and offset: Option<u32> (default 0). clamp_limit honours the silent-clamp contract; clamp_limit(Some(0)) → 0 returns an empty page without reads.
• Two-pass algorithm: pass 1 enumerate_email_ids (no parsing), pass 2 read_email_frontmatter only for [offset .. min(offset+limit, len)].
• filter_emails, parse_since, and the substring matchers are deleted from src/mcp.rs.
• No recursion, no walkdir — only read_dir of the folder root.
• Unit test (page_reads_at_most_limit_frontmatter_blocks) seeds 10 messages, asks for limit=3, offset=2, asserts exactly 3 frontmatter reads via the thread-local counter and the slice 8/7/6.
• Filename sort regression test (descending_sort_matches_reverse_insertion_order) exercises 100 messages with mixed-character slugs (digits, lowercase, uppercase, punctuation -/_).
• cargo test, cargo clippy --all-targets -- -D warnings, cargo fmt -- --check clean locally.

S2-2: JSON response shape — inbox vs. sent field divergence

• Two separate row structs (InboxListRow, SentListRow) — clean, easy to reason about. read is structurally absent from sent rows (struct doesn't define the field), not just null — sent_rows_omit_read_key parses the JSON and asserts key absence on the parsed object.
• delivery_status pass-through reads OutboundFrontmatter.delivery_status via the partial EmailListFm decoder; fallback to empty string only on malformed frontmatter.
• Empty mailbox / empty page returns "[]" (verified by empty_mailbox_returns_empty_array_no_reads, missing_mailbox_dir_returns_empty_array, offset_beyond_end_returns_empty_array, and the integration test mcp_email_list_empty_mailbox_returns_empty_json_array).
• Existing mcp_email_list_and_read integration test rewritten to assert against the JSON shape and demonstrate the client-side read == false filter.
• inbox_rows_carry_read_key confirms inbox rows carry read and not delivery_status.

S2-3: 10k-message perf test + sort-order regression test

• ten_thousand_message_page_50_reads_at_most_50_blocks seeds 10k inbox files and asserts reads ≤ 50 on a limit=50, offset=0 call. Wall-clock is logged, soft-asserted at 5000ms.
• descending_sort_matches_reverse_insertion_order covers the diverse-slug sort-order pin.
• Tests run on a non-root cargo-test runner using tempfile::TempDir.
• PARTIAL: the sprint plan suggested "New test in tests/integration.rs (or tests/email_list_perf.rs if a dedicated file is cleaner)". Both new tests live in the src/mcp.rs::email_list_tests unit-test module instead. The instrumentation works fine (the thread-local counter is wired correctly for the unit-level list_email_page_json call), but the chosen placement diverges from the criterion. Non-blocker.
• PARTIAL: the plan's wall-time soft bound was "e.g. 500ms — 10× the design target"; the PR uses 5000ms (100×). The justification ("CI runners with cold disk + debug builds") is reasonable but loose enough that an order-of-magnitude regression would slip through. Tightening to ~500–1000ms would be more useful.

S2-4: workflows.md polling pattern + mcp-tools.md email_list row

• workflows.md triage / mark-all-read / "filter by sender since a date" sections rewritten to show JSON.parse(email_list(...)) + client-side filter, with the verbatim "we no longer ask aimx to filter" note.
• mcp-tools.md email_list row rewritten with the new parameters, JSON shape, polling-then-direct-read worked example, and a paging-deeper example.
• book/mcp.md email_list section updated; book/mailboxes.md updated to reflect client-side filtering.
• book/hooks.md reference is generic (email_list / email_read) — no stale filter args.
• No call-site references to the dropped unread: / from: / since: / subject: filters anywhere outside future release notes.

Test Coverage

Coverage is good — every algorithmic branch has a corresponding test:

• clamp_limit enumerated for None, 0, 50, 200, 201, u32::MAX.
• Bundle-vs-flat enumeration covered by bundle_dir_contributes_one_id_to_listing (also confirms sibling attachments inside a bundle are not listed as separate ids).
• Empty / missing / past-end mailboxes covered with explicit fm_read_count_reset() == 0 assertions to confirm the early-return paths.
• Pagination covered both at unit level and integration level.

Gaps worth noting (none are blockers):

• No test exercises read_email_frontmatter returning Ok(None) when a bundle's inner .md is missing. The code path exists (line 1368) and the consequence is silently skipping the row, which can produce a page shorter than limit even when the sorted list has more entries. A unit test that seeds a bundle dir without its inner .md and asserts the surrounding rows still surface would pin this contract.
• No test covers read_email_frontmatter returning Ok(None) when splitn(3, "+++").parts.len() < 3 (a .md file without proper frontmatter delimiters). The behaviour is graceful (skip, don't error), but unverified.
• The empty-delivery_status fallback (unwrap_or_default() → "") on a malformed sent row is unverified.

Potential Bugs

None found that affect correctness on well-formed aimx-written frontmatter. A couple of sub-blocker observations:

1. Bundle-row skip can produce a short page (src/mcp.rs:1311-1314, :1328-1331). When read_email_frontmatter returns Ok(None) (missing inner .md or non-frontmatter content), the row is silently dropped. The slice [offset..end] is fixed before iteration, so a missing inner file shrinks the response without backfilling from the next id. Agents that page on len(rows) == limit would not infer "more pages exist" correctly. aimx never writes such a file, but disk corruption or a half-written bundle could trigger it. Non-blocker — graceful degradation is the right call here, but worth documenting in the function comment so a future maintainer doesn't "fix" it by backfilling.

2. enumerate_email_ids skips symlinks silently (src/mcp.rs:1264-1281). entry.file_type() does not follow symlinks, so a symlinked bundle dir or symlinked .md is silently dropped (not is_dir() and not is_file()). aimx never writes symlinks here, and the conservative behaviour is arguably the right security choice (no path-escape risk via read_email_frontmatter since the symlink is never followed). Non-blocker, with caveat — if any operator ever copies their inbox via cp -a or restores from a backup tool that produces symlinks, those messages will quietly disappear from listings. Worth a one-line comment explaining the policy.

3. choose_id fallback to frontmatter id (src/mcp.rs:1350-1356). The fallback runs only when the on-disk filename stem cannot decode to UTF-8 (path.file_stem().and_then(|f| f.to_str()) returned None). That path is already filtered out by enumerate_email_ids, which only pushes ids derived from to_str(). So the fallback is unreachable from the enumerate path — filename_stem is always non-empty when it gets here. Not a bug, but the comment claims a defence that the implementation already prevents upstream. Cosmetic.

Security Issues

No new attack surface. The MCP-side authorize_mailbox pre-flight is preserved on email_list, so the existing path-canonicalisation and ownership boundary stays intact. The new code reads files via plain std::fs::read_to_string on paths assembled from read_dir entries within the mailbox dir — no user-controlled path construction. The +++ splitter is straightforward and bounded by splitn(3), so a body containing +++ does not break parsing.

Code Quality

• EmailListFm (the partial frontmatter projection) is a nice optimisation — it avoids decoding the full InboundFrontmatter (with attachments arrays, references vectors, etc.) on the listing path. Worth keeping.
• The thread-local FM_READ_COUNT instrumentation is #[cfg(test)]-gated and only increments inside read_email_frontmatter. Tight, won't bleed into production.
• Folder::Inbox / Folder::Sent branches in list_email_page_json duplicate the iteration loop. Could be factored, but it's two short branches and the row-struct types differ — leaving as-is is fine.
• clamp_limit(Some(0)) → 0 is the right behaviour, and the test pins it. Just be aware: an agent that polls with limit=0 gets a perpetual empty page; the schema description doesn't call this out.

Alignment with PRD

• FR-B1 (parameter set) ✓
• FR-B2 (algorithm) ✓ — implementation matches the five-step recipe.
• FR-B3 (JSON shape divergence) ✓
• FR-B4 (single read_dir, no recursion) ✓
• FR-F2 (mcp-tools.md row rewrite) ✓
• FR-F3 (workflows.md polling pattern) ✓ — the verbatim "we no longer ask aimx to filter; we list a page and the agent decides" appears in workflows.md:11.
• §7 perf NFR (page-50 < 50ms cold on 10k) — read-count bound is hard-asserted; wall-time bar is documented in test prose but the assertion is 5000ms (loose).

The PR doesn't touch email_list's load_config() / authorize_mailbox path, so non-root agents can still hit the same 0640 root:root /etc/aimx/config.toml issue that motivated Sprint 1's mailbox_list fix. PRD §7 only mandates non-root access for mailbox_list; email_list access is not in scope here. Confirmed against PRD §7 — no divergence.

Summary and Recommended Actions

Overall verdict: Needs minor fixes — none of the findings are correctness-affecting on aimx-written frontmatter, but the loose perf bound and test placement deserve a touch.

Blockers: None.

Non-blockers:

1. Tighten the wall-time soft bound from 5000ms toward ~500–1000ms so a real perf regression is caught (S2-3).
2. Move the perf + sort-order tests from src/mcp.rs::email_list_tests to tests/integration.rs or tests/email_list_perf.rs to match S2-3's stated placement, OR document in the sprint plan that the unit-level placement is intentional. (The thread-local counter would need to become pub if moved.)
3. Add a one-line comment in enumerate_email_ids explaining the symlink-skip policy, and a one-line comment in list_email_page_json explaining that bundle-row skips can shrink the page below limit (so a future maintainer doesn't backfill).
4. Optional: a unit test that seeds a bundle dir without its inner .md and asserts surrounding rows still surface — pins the "graceful skip" contract.

Nice-to-haves:

• The fallback in choose_id is unreachable given how enumerate_email_ids filters; the comment overstates the defence. Either delete the fallback (let the empty stem propagate, which is itself unreachable) or rephrase the comment.
• Tests for read_email_frontmatter returning Ok(None) on missing-frontmatter content (splitn(3) < 3) and on malformed sent delivery_status.

The implementation is in good shape and the algorithmic invariants are correctly pinned. Once the non-blockers above are addressed (or explicitly tabled into the sprint backlog), this is mergeable.

Re-review — Cycle 2

Verified 57a8aa0 against the previous review. All four addressed items check out, the two cheap nice-to-haves were also picked up, and the two tabled items have proper rationale + backlog entries.

Resolved

1. Wall-time soft bound tightened (5000ms → 500ms). src/mcp.rs:1888 now asserts elapsed.as_millis() < 500. Comment correctly frames it as ~10x the <50ms cold-cache target. The 10k-message test passes locally on a debug build, so an order-of-magnitude regression will now trip.
2. Symlink-skip policy comment at src/mcp.rs:1265-1269 — concise, accurate, explicitly warns a future maintainer not to "fix" the silent drop, and notes the read_email_frontmatter path-escape posture.
3. "Page may be shorter than limit" contract at src/mcp.rs:1294-1300 — clearly states the slice is fixed before iteration, that Ok(None) rows drop without backfilling, and that this is intentional graceful degradation. Pairs well with the new bundle-skip test below.
4. Unit test for bundle-without-inner-md skip (bundle_without_inner_md_is_skipped_surrounding_rows_surface) — seeds two flat siblings around an empty bundle dir, asserts the broken row drops and the neighbours surface in correct descending order. Passes.

Plus the two cheap nice-to-haves:

5. splitn(3) < 3 malformed-frontmatter skip test (missing_frontmatter_delimiters_skips_row) — a .md without +++ delimiters is silently skipped. Passes.
6. Empty delivery_status fallback test (sent_row_with_missing_delivery_status_falls_back_to_empty_string) — sent row missing the field surfaces as "" (verbatim, not null). Passes.

Tabled (acceptably)

• Test placement for the perf + sort-order tests — reasonable rationale (binary-only crate, no lib.rs, #[cfg(test)] thread-local counter cannot be reached from tests/ integration tests via assert_cmd). Backlog entry added at docs/mcp-update-sprint.md:315 with the revisit condition. The previous review explicitly offered this as an acceptable alternative.
• Dead/unreachable choose_id fallback comment — cosmetic only. Backlog entry at docs/mcp-update-sprint.md:316.

Verification

• cargo test --bin aimx — 966 passed, 0 failed.
• cargo test --bin aimx email_list_tests — all 13 tests in the module pass, including the tightened 10k-message page-50 perf test.
• cargo clippy --all-targets -- -D warnings — clean.
• cargo fmt -- --check — clean.

No regressions introduced by 57a8aa0. New tests are well-scoped and the new comments are accurate.

Verdict

Ready to merge.

Recommended merge commit message

Rewrite email_list as paginated, JSON-shaped, index-free MCP tool

Replaces the filter-laden email_list (unread/from/since/subject) with a
two-pass, page-bounded algorithm: read_dir enumerates ids without
parsing, sort descending lexicographically (filenames are
YYYY-MM-DD-HHMMSS-<slug> so descending lex == reverse chronological),
then read frontmatter only for the page slice. Listing N rows reads at
most N frontmatter blocks regardless of mailbox size — pinned by a
10k-message test that asserts the page-50 call hits the disk for
exactly 50 frontmatter blocks under a tightened 500ms wall-time bound
(~10x the <50ms cold-cache design target).

EmailListParams loses unread/from/since/subject and gains
limit (default 50, max 200; values above silently clamp) and
offset (default 0). filter_emails and the old plaintext list_emails
helper are removed.

Response is JSON. Inbox rows: { id, from, to, subject, date, read }.
Sent rows: { id, from, to, subject, date, delivery_status } — read is
intentionally absent from sent rows since agents do not mark sent mail.
Empty mailbox returns the literal "[]". Pages may be shorter than
limit when a row's frontmatter is unreadable (Ok(None)); this is
documented and pinned by tests for missing inner .md, missing +++
delimiters, and missing delivery_status.

Docs (primer, references/mcp-tools.md, references/workflows.md,
book/mcp.md, book/mailboxes.md) rewritten to show the new shape and
the client-side filter pattern: list a page, filter on the agent side,
process. We no longer ask aimx to filter; the agent decides.