Add attachments to emailSend, matching emailReply - #2296
Conversation
Share the capability attachment schema and mapper with emailReply, keep one MIME message (and one daily send charge) for every allowed to, and fail oversize or unsafe filenames before the provider call. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe PR adds attachment support to ChangesEmail attachments
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant MCPCaller
participant emailSendCapability
participant sendOutboundEmail
participant EMAILBinding
MCPCaller->>emailSendCapability: Send attachment input
emailSendCapability->>emailSendCapability: Validate and convert attachments
emailSendCapability->>sendOutboundEmail: Pass outbound attachments
sendOutboundEmail->>EMAILBinding: Send one MIME message to allowed destinations
EMAILBinding-->>MCPCaller: Return sent message summary
Possibly related PRs
Merge Risk: ⚪ Minimal · up to Attachment support includes shared validation, outbound safety checks, delivery coverage, and documentation without an identified merge-blocking issue. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 6 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 63e008a. Configure here.
|
🔎 Preview deployed: https://kody-pr-2296.kody-a99.workers.dev Worker: Mocks:
|
Intent
Let
emailSendattach files the same wayemailReplyalready does, without opening a new send channel or quota.Why
Kent approved notify-self sends with optional attachments. Destinations still gate recipients. Size stays on the existing
email_message_bytescap. No new entitlement dimension.Summary
emailSendaccepts optionalattachments(filename,content_type,content_base64, max 10)emailReply— no forked validationtoemail_sends_per_daycharge per successful senddocs/use/email-primitives.mdmirror the reply attachment noteTesting
email-send.node.test.ts: no attachments unchanged, attachments forwarded, invalid shape / empty list / 11 files rejectedoutbound.workers.test.ts: attachments under cap, oversizeemail_message_bytes, path-traversal filename, multi-toone MIME + one daily chargetest:node(3226) andtest:workers(364) passedSystem changes
Medium risk: this extends the
emailSendcontract. Notify-self and destination gating are unchanged. Leave merge for Cole/Kent after CI.System recap — extends email (medium risk)
Mode: recap · Base:
main@29ceea9d· Head:63e008aaClassification: extends —
emailSendgains the same optional attachment contract asemailReply. Destination gating, one daily send charge, and notify-self are unchanged.Primitives touched
emailemailSendacceptsattachments, shared schema/mapper withemailReply, filename guardChange flow
emailSendvalidates attachments with the shared reply schema, thensendOutboundEmailbuilds one MIME message for every allowedtoand fails oversize before the provider.Before / after
Invariants
Per-user isolation and notify-self still hold: attachments never expand who can receive, and unverified or unknown
tostill fails the whole send.Summary by CodeRabbit