feat(email): mailbox folders, greylist review, and outbound drafts#97
feat(email): mailbox folders, greylist review, and outbound drafts#97Jacky040124 wants to merge 10 commits into
Conversation
Introduce mailbox classification on emails with migration, shared constants, query helpers, and draft-send lifecycle functions. Co-authored-by: Cursor <cursoragent@cursor.com>
Stop rejecting unknown senders at the worker; store them for review via notify and update whitelist empty-state copy. Co-authored-by: Cursor <cursoragent@cursor.com>
Route folder queries through mailbox helpers and support discarding inbound review emails into the untrust mailbox. Co-authored-by: Cursor <cursoragent@cursor.com>
Add draft CRUD/send APIs, extract sender resolution, and wire compose plus client helpers for saving and sending drafts. Co-authored-by: Cursor <cursoragent@cursor.com>
Add draft folder UX, discard/send-from-draft actions, race-safe loading state helpers, and hydration shells for sidebar and email page. Co-authored-by: Cursor <cursoragent@cursor.com>
Align folder filtering assertions with mailbox columns: untrust inserts set mailbox explicitly, inbox checks mailbox, and draft covers greylist review emails. Co-authored-by: Cursor <cursoragent@cursor.com>
gusye1234
left a comment
There was a problem hiding this comment.
Review Summary
Architecture is solid — approve with minor suggestions.
The mailbox column + greylist flow + draft send state machine are well-designed. Test coverage is thorough.
Minor Items (non-blocking):
-
Silent error swallowing in send route —
restoreClaimedDraftandmarkClaimedDraftUnknownfunctions swallow errors. If those DB calls fail, the draft stays stuck insendingstate forever. Consider logging failed restore/markUnknown calls. -
Send route length (~400 lines) — The draft state machine layered on top of direct-send has grown the file significantly. Future refactor splitting draft-send from direct-send would improve maintainability.
-
Migration gap — Emails with
direction = 'outbound'butstatus != 'sent'(failed old sends) will getmailbox = 'inbox'by default. Minor data integrity gap, unlikely to cause user-visible issues. -
Naming —
handleDiscardDraftoperates on inbound review emails, not user-composed drafts. Could mislead future devs. ConsiderhandleDiscardReviewEmail.
None of these block merge.
Route non-whitelisted inbound mail through a fail-closed triage task, and apply triage results through scoped mailbox state transitions so user-handled mail is not overwritten. Co-authored-by: Cursor <cursoragent@cursor.com>
Run email triage with a dedicated readonly execution profile across supported providers, and fail closed when a provider cannot enforce the boundary. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a local end-to-end script that exercises the main email triage branches and preserves test data for UI verification. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
gusye1234
left a comment
There was a problem hiding this comment.
Follow-up Review — New Commits (e49154d, a2fe1ab, bbc0435)
Reviewing the readonly triage workflow added since my last review on 2026-05-23.
What was added
Routes non-whitelisted inbound email through a fail-closed readonly agent triage session. The agent classifies email as untrust or draft_reply via structured JSON output. Multiple enforcement layers: Claude permission-mode denial, Codex read-only sandbox, OpenCode fail-closed, plus prompt-level instructions.
Findings
Major — Rate limiting on triage task creation (non-blocking)
Every non-whitelisted inbound email spawns a full LLM agent session. A spam flood could overwhelm the task queue and burn LLM credits. No deduplication or queue-depth guard exists before enqueue.
Recommendation: Add a guard checking total queued triage tasks before enqueuing, or batch triage for low-priority senders. Can be a follow-up ticket.
Minor — recoverStaleEmailTriageApplies uses emails.createdAt not apply-start time
The 1-hour staleness check compares against createdAt (when the email arrived), not when triage-apply started. If an email sat in draft before triage ran, it could be recovered prematurely. Unlikely in practice since triage is enqueued immediately, but semantically off. Consider using updatedAt or checking the triage task's completion time.
Minor — enqueueEmailTriageTask returns null silently on multiple failure paths
When triage can't be enqueued (R2 unavailable, empty body), the email is orphaned in draft with no logging of why. Consider a warning log.
Minor — Failed triage task leaves email in draft with no owner notification
Correct fail-closed behavior (don't auto-trust), but no mechanism alerts the owner that manual review is needed. Consider a dashboard indicator or notification.
Security Assessment
The readonly enforcement is defense-in-depth and sound for a first iteration:
- Claude: control_requests denied + no CLI tool docs injected in triage context
- Codex: OS-level read-only sandbox (CLI) / approval rejection (MCP)
- OpenCode: fails closed entirely (rejects triage tasks)
- Fail-closed on every error path — invalid output, crashes, timeouts, DB failures all leave email in draft
The practical attack surface for bypassing readonly requires the LLM to spontaneously construct the exact CLI command without being told about it — acceptable risk given controlled workspace.
Verdict
Solid implementation. The fail-closed principle is genuinely maintained across all failure paths. Tests are comprehensive. Approve with the rate-limiting concern tracked as a follow-up.
Summary
mailboxcolumn on emails (inbox, sent, draft, untrust) with a D1 migration and shared query/helpers for folder-based listing and mailbox updates.Test plan
pnpm typecheckpnpm test0033_email_mailbox.sqlbefore deployMade with Cursor