Design: Full API parity for CLI and MCP

[seanb4t]

Comprehensive design for full CRUD parity between CLI and MCP server with Fastmail JMAP/CardDAV/CalDAV APIs.

Includes 19 new CLI commands, 13 new MCP tools, 4 new resources, and 5-phase implementation plan starting with mailbox and mail operations.

[seanb4t]

PR #5 Review Summary: "Design: Full API parity for CLI and MCP"

Scope: 50 files, +8700/-219 lines — CLI commands, JMAP protocol, MCP tools/resources, service layer, search parser

---

Critical Issues (3)

1. SetKeywords uses false instead of null to remove JMAP keywords

pkg/fastmail/mail.go — Per RFC 8620 §5.3, removing a key from a JMAP map requires setting it to null, not false. Per RFC 8621 §4.1.1, keyword values MUST be true. The current code sends "keywords/$seen": false for --unread, which is protocol-noncompliant. Fix: map false to nil.

2. MailService.Delete permanently destroys email on transient Trash-lookup failure

pkg/fastmail/mail.go:394-399 — When resolveMailbox(ctx, accountID, "Trash") fails (network timeout, 401, 429, 500), the code falls through to s.destroy() — permanent, irrecoverable deletion. Should only fall through for "mailbox not found", not transient errors.

3. Silent type coercion in MCP argument helpers

mcp/tools_mail.go:586-642 — getStringArg, getUint64Arg, getBoolArg, getStringSliceArg silently return defaults when the arg is present but wrong type. An agent sending {"id": 12345} (int) gets the misleading error "id is required" instead of a type mismatch error. getStringSliceArg also silently drops non-string array elements.

---

Important Issues (5)

4. --enable/--disable vacation flags silently conflict

cli/vacation.go, mcp/tools_mail.go — --disable silently overwrites --enable due to execution order. No mutual exclusivity validation (unlike mail flag's read/unread).

5. MCP mail_flag handler lacks mutual exclusivity validation

mcp/tools_mail.go — CLI validates --read/--unread mutual exclusivity; MCP handler does not. Agent can pass both, getting undefined last-write-wins behavior.

6. convertEmails writes to stderr instead of returning errors

pkg/fastmail/mail.go:565-602 — Invalid ReceivedAt timestamps produce stderr warnings invisible to MCP clients and JSON consumers. Violates project convention of using samber/oops.

7. MCP resources return fake success instead of errors

mcp/resources.go:253-271 — handleContacts and handleCalendarToday return 200-equivalent success with error text when backend isn't configured. Agent parses "error message" as data. Should return actual errors.

8. SearchWithFilter mutates caller's filter map

pkg/fastmail/mail.go — Public API modifies the filter map in-place when resolving mailbox names to IDs. Callers retaining a reference see unexpected mutations.

---

Suggestions (4)

9. MailService.Move makes redundant Get call

pkg/fastmail/mail.go — Fetches email to validate existence, then discards result (_ = email). The Email/set call would return NotUpdated anyway. Wastes an API round-trip.

10. getAccountID errors lack wrapping context

Multiple files — ~20+ call sites return getAccountID errors unwrapped. When debugging "getting session: connection refused", you can't tell which operation triggered it.

11. internal/jmap/client.go uses fmt.Errorf not samber/oops

Inconsistent with the rest of the codebase at the lowest-level network layer where structured errors matter most.

12. Inconsistent --force behavior

mailbox delete without --force returns nil (exit 0 — success). mail delete without --force returns an error (non-zero exit). Scripts checking $? get wrong signal from mailbox delete.

---

Test Coverage Gaps

Gap	Severity
MCP tool handlers are schema-only tests (no handler invocation)	High
CalendarService has zero tests (pkg/fastmail/calendar.go)	High
SetKeywords only tests true values, not keyword removal	High
MCP mail_flag with contradictory flags untested	Medium
VacationService.Set with empty options untested	Medium
Search parser: is:read, is:draft, is:answered unhandled	Low

---

Strengths

• Well-structured three-layer architecture (internal → pkg → cli/mcp)
• Thorough JMAP builder + response deserialization tests
• Excellent error-path coverage at service layer (NotCreated/NotUpdated/NotDestroyed)
• Smart two-stage delete (move to Trash, then permanent if already in Trash)
• Protected mailbox deletion prevents deleting inbox/drafts/sent/trash/junk
• Search parser is clean and extensible with solid test coverage

---

Recommended Action

1. Fix critical issues first — keyword removal bug (feat(cli): add mail read command (fc-h2at) #1) and delete-on-transient-error (feat(cli): add mail read command (fc-h2at) #2) affect data integrity
2. Add MCP handler invocation tests — the schema-only tests leave significant integration gaps
3. Address important validation gaps (Improve auth handling and CardDAV defaults #4, Design: Full API parity for CLI and MCP #5) before MCP goes to production
4. Consider the suggestions for a follow-up PR

---

🤖 Generated with Claude Code using pr-review-toolkit