Add storage-only email primitives - #284
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a first-class email subsystem: DB schema and repo, MIME parsing, address/subject utilities, sender policy evaluation, inbound/outbound handlers (with Cloudflare EMAIL binding support and REST fallback), MCP email domain and capabilities, tests, docs, and necessary env/wrangler/type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant External as External Mail
participant Worker as Kody Worker
participant Parser as MIME Parser
participant Repo as D1 Repository
participant Policy as Policy Evaluator
External->>Worker: ForwardableEmailMessage
activate Worker
Worker->>Parser: parseForwardableEmailMessage(message)
Parser-->>Worker: ParsedInboundEmail
Worker->>Repo: getEmailInboxAddressByAddress()/getByReplyTokenHash()
Repo-->>Worker: InboxAddress or null
Worker->>Repo: listEmailSenderPolicies(userId)
Repo-->>Worker: PolicyRecords
Worker->>Policy: evaluateSenderPolicy(from/envelope/replyToken)
Policy-->>Worker: policyDecision
Worker->>Repo: findThreadForMessage(...) or createEmailThread(...)
Repo-->>Worker: ThreadRecord
Worker->>Repo: insertEmailMessageWithAttachments(...)
Repo-->>Worker: MessageRecord
Worker->>Repo: insertEmailDeliveryEvent(event)
Repo-->>Worker: EventRecord
Worker->>External: Accept or Reject (ack)
deactivate Worker
sequenceDiagram
participant MCP as MCP Client
participant Handler as Capability Handler
participant Repo as D1 Repository
participant Email as Cloudflare EMAIL Binding
MCP->>Handler: email_send(input)
activate Handler
Handler->>Repo: getVerifiedSenderIdentity(userId, from)
Repo-->>Handler: SenderIdentity or error
Handler->>Repo: insertEmailMessage(direction: outbound)
Repo-->>Handler: MessageRecord
Handler->>Repo: insertEmailDeliveryEvent(send_requested)
Repo-->>Handler: EventRecord
Handler->>Email: EMAIL.send(payload)
alt Binding success
Email-->>Handler: { messageId / message_id }
else Binding absent or error
Handler->>Handler: sendCloudflareEmail(REST fallback)
Handler-->>Handler: fallback result
end
Handler->>Repo: updateEmailMessageDelivery(sent|failed)
Repo-->>Handler: UpdateResult
Handler->>Repo: insertEmailDeliveryEvent(sent|failed)
Repo-->>Handler: EventRecord
Handler-->>MCP: { message summary, provider_message_id?, status, error? }
deactivate Handler
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 17 minutes and 31 seconds.Comment |
kentcdodds
marked this pull request as ready for review
April 30, 2026 05:09
Contributor
|
🔎 Preview deployed: https://kody-pr-284.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
packages/worker/src/email/outbound.workers.test.ts (1)
12-64: ⚡ Quick winAdd coverage for the REST fallback path.
This test only exercises the native
EMAIL.sendbranch, but the PR also introduces a fallback flow. A second test that forces the binding path to fail or be absent would keep the new branch from regressing.♻️ Suggested follow-up test
+test('sendOutboundEmail falls back when EMAIL is unavailable', async () => { + // Arrange an env without EMAIL, or mock the native send path to fail. + // Assert the REST-backed path still returns a sent result and persists delivery state. +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/email/outbound.workers.test.ts` around lines 12 - 64, Add a second unit test alongside the existing "sendOutboundEmail uses SendEmail binding and stores sent delivery state" that forces the SendEmail binding path to fail so the REST fallback is used: call sendOutboundEmail with an env where EMAIL is undefined or where env.EMAIL.send throws an error (e.g., replace env.EMAIL with undefined or a send that throws), mock/spy the REST transport used by sendOutboundEmail (the HTTP/fallback client invoked in that branch) to return a providerMessageId, then assert that the REST path sent the message and that the stored message has processingStatus 'sent' and providerMessageId matching the mocked REST response; reference sendOutboundEmail, env.EMAIL, and any fallback HTTP client or function used in the implementation when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/email/inbound.ts`:
- Around line 74-80: The code computes policyDecision from evaluateSenderPolicy
and emits a 'rejected' delivery event but never sets the SMTP rejection on the
inbound message; update the inbound handler(s) that use policyDecision (the
block after evaluateSenderPolicy and the additional handler range that also
checks policyDecision) to call message.setReject(...) when policyDecision ===
'rejected' (passing an appropriate reject reason/string) before ending/accepting
the SMTP transaction, and ensure the same behavior occurs in both locations
where you currently record the rejected event so the sender actually receives an
SMTP rejection.
- Around line 128-133: The current object sets the error field to policyDecision
reasons (decision.reasons) even for accepted/quarantined messages; change the
logic so error is only populated for real failures (e.g., unknown inbox alias or
when processingStatus indicates failure/rejection), otherwise set error to null;
specifically update the assignment of the error property (the object containing
policyDecision, processingStatus, providerMessageId, error) to use inboxAddress
and processingStatus (or decision.action/outcome if available) to decide whether
to set decision.reasons.join(', ') or null, keeping the `unknown inbox alias:
${recipient}` value only when inboxAddress is falsy.
In `@packages/worker/src/email/outbound.ts`:
- Around line 99-120: sendViaRestFallback currently omits reply/threading
metadata so REST sends differ from sendViaBinding; update sendViaRestFallback to
pass replyTo (e.g., replyTo: input.replyTo ?? undefined) and include any custom
headers used for threading (e.g., 'In-Reply-To' and 'References') in the payload
you pass to sendCloudflareEmail (mirror the same header names/values
sendViaBinding uses), and make the same change for the second fallback
occurrence mentioned (lines ~228-249) so both REST fallback paths preserve
reply-to and threading headers.
In `@packages/worker/src/email/parser.ts`:
- Around line 99-107: parseReplyToken currently only reads the
X-Kody-Reply-Token header while routing (findReplyTokenHash) also accepts
X-Reply-Token, causing parsed tokens to be missing; update parseReplyToken to
check both header names (e.g., use getHeader(headers, 'X-Kody-Reply-Token') ||
getHeader(headers, 'X-Reply-Token')) before falling back to scanning toAddresses
for +reply-... matches so reply_token policy matching succeeds.
In `@packages/worker/src/email/policy.ts`:
- Around line 94-95: The reply token is currently assigned as a lowercased raw
string in replyTokenHash but must be the hashed representation used for policy
comparisons; update the assignment of replyTokenHash to produce the same hash
used elsewhere in the pipeline (use the existing hash utility/function used to
create reply_token hashes) so matchSenderPolicyRule compares hash-to-hash;
ensure you still handle null/undefined inputs and then iterate rules via
sortPolicyRules(input.rules) as before.
In `@packages/worker/src/email/repo.ts`:
- Around line 679-953: The message and attachments are persisted separately
(insertEmailMessage and insertEmailAttachments) which can leave partial state on
failure; add a single atomic repository operation (e.g.,
insertEmailMessageWithAttachments) that takes the same message payload plus
attachments and performs both inserts inside one DB transaction (or a
transaction-emulating batch), using the same generated message id and timestamp,
and rolling back/compensating (delete the message) if any attachment insert
fails; update callers to use this new function instead of calling
insertEmailMessage then insertEmailAttachments separately.
- Around line 482-533: The upsertEmailSenderPolicy logic builds a row reusing
existing.id but always issues a plain INSERT, which will conflict when a record
exists; change this to perform an UPDATE when existing is truthy (or use an
INSERT ... ON CONFLICT(id) DO UPDATE) instead of inserting, updating fields
(inbox_id, package_id, kind, value, effect, enabled, updated_at) for the record
with id = existing.id; locate the code around the email_sender_policies
query/row creation (variables existing, row and the input.db.prepare(...) that
does INSERT) and replace the INSERT branch with an UPDATE (or an upsert-style
INSERT ... ON CONFLICT) to avoid primary key violations and ensure updated_at is
set.
- Around line 688-740: insertEmailMessage currently writes nullable values for
columns that are NOT NULL in the DB (notably from_address, subject, raw_size),
which can cause runtime constraint violations; update the row construction in
insertEmailMessage so non-nullable columns get safe defaults when omitted (e.g.,
set from_address to an empty string when input.message.fromAddress is
null/undefined, subject to empty string when input.message.subject is
null/undefined, and raw_size to 0 when input.message.rawSize is null/undefined)
while keeping the existing JSON defaults for address arrays and headers.
In `@packages/worker/src/email/test-schema.ts`:
- Around line 3-125: The test schema for email tables diverges from the
migration (0030-email-primitives.sql) and allows states the production schema
rejects; update the test DDL so it matches the migration exactly (or load the
migration SQL as the single source-of-truth) by making these concrete changes:
in email_inboxes set description to NOT NULL if migration requires it; in
email_threads set subject_normalized NOT NULL per migration; in email_messages
set from_address NOT NULL, change headers_json default to the same literal used
in the migration, add the missing policy_decision enum values (include 'sent'
and 'failed'), and enforce the same NOT NULL/DEFAULT constraints for
processing_status and related columns; in email_delivery_events make provider
and detail_json NOT NULL if migration does; and add the same foreign key
constraints and unique/partial indexes (e.g., idx_email_sender_identities unique
constraint on (user_id,email), idx_email_sender_policies unique constraints and
any partial-unique behavior) so the test DDL and migration DDL are identical or
loaded from the same SQL source.
In `@packages/worker/src/email/types.ts`:
- Around line 96-102: The EmailInboxRecord and AttachmentRecord type definitions
are out of sync with the mapper functions: mapInboxRow always normalizes
description to an empty string and mapAttachmentRow always defaults size to 0,
so make those fields non-nullable to reflect runtime values—update
EmailInboxRecord to have description: string (not string | null) and update the
AttachmentRecord (lines ~191-198) to have size: number (not number | null); keep
packageId or other fields unchanged and run typechecks to ensure callers drop
redundant null-checks.
In `@packages/worker/src/mcp/capabilities/email/email-inbox-create.ts`:
- Around line 29-45: The inbox and alias creation must be made atomic: wrap the
calls to createEmailInbox and createEmailInboxAddress (and the hashReplyToken
call that supplies replyTokenHash) in a single database transaction or move
logic into a repository method (e.g., createEmailInboxWithAddress) that performs
both inserts within one transaction and rolls back on error; ensure you still
generate and return the replyToken on success and propagate/handle errors on
failure so no orphaned inbox can exist if address insertion fails.
In `@packages/worker/src/mcp/capabilities/email/email-reply.ts`:
- Around line 49-53: The current reply header logic sets inReplyToHeader to
original.id when original.messageIdHeader is null, producing a non-email
identifier; change the assignment so inReplyToHeader is only set when
original.messageIdHeader exists (i.e., omit or leave undefined if null) and
continue to populate references using original.references plus
original.messageIdHeader only when present; update the inReplyToHeader
assignment in the email-reply code (look for the inReplyToHeader,
original.messageIdHeader, original.id, and references usage) to avoid falling
back to the DB id.
In `@packages/worker/src/mcp/capabilities/email/email-sender-revoke.ts`:
- Around line 19-45: The handler for this capability fails to pass
package-scoped revocations because inputSchema and the call to
disableEmailSenderPolicy only handle inbox_id; update the inputSchema to accept
an optional package_id (string, .optional(), describe it) and then pass
packageId: args.package_id ?? null into the disableEmailSenderPolicy call
(alongside inboxId). Keep the existing normalization and error checks unchanged.
---
Nitpick comments:
In `@packages/worker/src/email/outbound.workers.test.ts`:
- Around line 12-64: Add a second unit test alongside the existing
"sendOutboundEmail uses SendEmail binding and stores sent delivery state" that
forces the SendEmail binding path to fail so the REST fallback is used: call
sendOutboundEmail with an env where EMAIL is undefined or where env.EMAIL.send
throws an error (e.g., replace env.EMAIL with undefined or a send that throws),
mock/spy the REST transport used by sendOutboundEmail (the HTTP/fallback client
invoked in that branch) to return a providerMessageId, then assert that the REST
path sent the message and that the stored message has processingStatus 'sent'
and providerMessageId matching the mocked REST response; reference
sendOutboundEmail, env.EMAIL, and any fallback HTTP client or function used in
the implementation when adding the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a812e4f0-c6ee-4484-8469-f39e6d14185b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
docs/contributing/setup-manifest.mddocs/use/email-primitives.mdpackage.jsonpackages/worker/migrations/0030-email-primitives.sqlpackages/worker/src/email/address.node.test.tspackages/worker/src/email/address.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/parser.node.test.tspackages/worker/src/email/parser.tspackages/worker/src/email/policy.node.test.tspackages/worker/src/email/policy.tspackages/worker/src/email/repo.tspackages/worker/src/email/test-schema.tspackages/worker/src/email/types.tspackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/email/domain.tspackages/worker/src/mcp/capabilities/email/email-domain.node.test.tspackages/worker/src/mcp/capabilities/email/email-inbox-create.tspackages/worker/src/mcp/capabilities/email/email-inbox-list.tspackages/worker/src/mcp/capabilities/email/email-message-get.tspackages/worker/src/mcp/capabilities/email/email-message-list.tspackages/worker/src/mcp/capabilities/email/email-policy-get.tspackages/worker/src/mcp/capabilities/email/email-reply.tspackages/worker/src/mcp/capabilities/email/email-send.tspackages/worker/src/mcp/capabilities/email/email-sender-approve.tspackages/worker/src/mcp/capabilities/email/email-sender-revoke.tspackages/worker/src/mcp/capabilities/email/shared.tspackages/worker/src/package-registry/types.tspackages/worker/src/package-runtime/module-graph.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonc
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| db: input.env.APP_DB, | ||
| userId: input.userId, | ||
| inboxId: original?.inboxId ?? input.inboxId ?? null, | ||
| subjectNormalized: subject.toLowerCase(), |
Contributor
There was a problem hiding this comment.
Inconsistent subject normalization between inbound and outbound paths
Medium Severity
The inbound path uses normalizeSubject(parsed.subject) which strips Re:/Fwd: prefixes, collapses whitespace, and lowercases. The outbound path uses subject.toLowerCase() only. This means the same conversation thread stores different subjectNormalized values for inbound vs outbound messages — e.g. inbound stores "hello" while the outbound reply stores "re: hello". This data inconsistency will break any future subject-based thread correlation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ed0a7e1. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/outbound-email.ts`:
- Around line 20-31: The headers validator currently accepts any object
(including class instances or Date), so update the guard in the function
handling `value`/`context`/`fail` to only accept plain object literals: after
confirming value is an object and not an array, also check its prototype (e.g.
ensure Object.getPrototypeOf(value) === Object.prototype ||
Object.getPrototypeOf(value) === null) and call fail('Expected headers object',
context.path) if not plain; keep the existing iteration over
Object.entries(value) and string-value checks for `headerValue` into the
`headers` map.
In `@packages/worker/src/app/email/cloudflare-email.ts`:
- Around line 60-61: The current send flow is logging message.headers which may
expose sensitive metadata; update the logging in the Cloudflare email send
routine so it no longer logs raw message.headers (refer to the message.headers
usage) — either filter to an allowlist of safe header names before logging or
redact header values (e.g., replace values with "[REDACTED]") and then log the
sanitized object; locate the email send function in cloudflare-email.ts where
replyTo and headers are set and replace the direct log of message.headers with
the sanitized/allowlisted version.
In `@packages/worker/src/email/inbound.ts`:
- Around line 64-148: The code persists messages using a synthetic
unknownInboxUserId when inboxAddress is missing, allowing unbound/unknown
aliases to create threads/messages; modify the flow in the inbound handler to
early-reject or drop messages when inboxAddress is null (before calling
createEmailThread and insertEmailMessageWithAttachments), and avoid using
unknownInboxUserId for any persistent inserts; use the inboxAddress check
(inboxAddress, unknownInboxUserId) to return/log/quarantine immediately and only
call createEmailThread/insertEmailMessageWithAttachments when a real
inboxAddress/inbox.id and userId are resolved.
In `@packages/worker/src/email/parser.ts`:
- Around line 95-104: parseReplyToken only extracts "+reply-<token>" but must
also accept "kody-r-<token>" recipients to match findReplyTokenHash routing;
update parseReplyToken (and its localPart regex) to test for both patterns —
e.g. check localPart.match(/(?:\+reply-|kody-r-)([a-z0-9_-]+)/i) — and return
the captured token so parsed.replyToken is populated for messages sent to
kody-r-<token> addresses.
In `@packages/worker/src/email/repo.ts`:
- Around line 453-502: The current upsert builds a speculative row and returns
mapSenderIdentityRow(row), which can be incorrect under concurrent upserts;
change the flow so after the INSERT ... ON CONFLICT executes you read back the
actual persisted row (either by adding a RETURNING * to the INSERT and using the
returned row from input.db.prepare(...).first(...) or by running a SELECT for
the row by (user_id, email) after .run()), and then pass that persisted row into
mapSenderIdentityRow instead of the locally constructed `row`; update references
to `row` to use the fetched persisted record (e.g., persistedRow) so id and
created_at reflect what was stored.
- Around line 690-725: createEmailThread currently sets row.subject_normalized
to null when input.subjectNormalized is absent, but the DB schema defines
subject_normalized as TEXT NOT NULL DEFAULT '', causing inserts to fail; update
the code so subject_normalized is set to an empty string when absent (e.g., use
input.subjectNormalized ?? '' in the row object and bind that value) so the
prepared INSERT for email_threads binds a non-null string for subject_normalized
instead of null.
In `@packages/worker/src/mcp/capabilities/email/email-inbox-create.ts`:
- Around line 28-43: The input allows whitespace-only names because validation
uses z.string().min(1) but the handler stores args.name.trim(); update the
validation on inputSchema.name to reject whitespace-only values (for example use
z.string().trim().min(1) or z.string().min(1).refine(s => s.trim().length > 0));
keep the handler’s use of args.name.trim() (in the handler where
createEmailInboxWithAddress is called) consistent with the new schema so
persisted inbox names cannot end up empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b41f09c9-4196-4b92-a9ae-4b0c12e9fada
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
packages/shared/src/outbound-email.tspackages/worker/migrations/0030-email-primitives.sqlpackages/worker/package.jsonpackages/worker/src/app/email/cloudflare-email.tspackages/worker/src/email/address.node.test.tspackages/worker/src/email/address.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/parser.node.test.tspackages/worker/src/email/parser.tspackages/worker/src/email/policy.node.test.tspackages/worker/src/email/policy.tspackages/worker/src/email/repo.tspackages/worker/src/email/test-schema.tspackages/worker/src/email/types.tspackages/worker/src/mcp/capabilities/email/email-inbox-create.tspackages/worker/src/mcp/capabilities/email/email-reply.tspackages/worker/src/mcp/capabilities/email/email-sender-revoke.tspackages/worker/src/mcp/capabilities/email/shared.ts
✅ Files skipped from review due to trivial changes (5)
- packages/worker/package.json
- packages/worker/src/email/policy.node.test.ts
- packages/worker/src/mcp/capabilities/email/email-sender-revoke.ts
- packages/worker/src/email/address.node.test.ts
- packages/worker/src/email/address.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/worker/src/email/test-schema.ts
- packages/worker/src/email/inbound.workers.test.ts
- packages/worker/src/mcp/capabilities/email/shared.ts
- packages/worker/src/mcp/capabilities/email/email-reply.ts
- packages/worker/src/email/outbound.workers.test.ts
- packages/worker/src/email/outbound.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/src/email/test-schema.ts (1)
30-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing
email_inbox_addresses.inbox_idforeign key to match migration behavior.Without the FK, tests can pass with orphan inbox-address rows and miss
ON DELETE CASCADEbehavior expected in production.Proposed fix
`CREATE TABLE IF NOT EXISTS email_inbox_addresses ( id TEXT PRIMARY KEY, inbox_id TEXT NOT NULL, user_id TEXT NOT NULL, address TEXT NOT NULL UNIQUE, local_part TEXT NOT NULL, domain TEXT NOT NULL, reply_token_hash TEXT, enabled INTEGER NOT NULL DEFAULT 1 CHECK (enabled IN (0, 1)), created_at TEXT NOT NULL, - updated_at TEXT NOT NULL + updated_at TEXT NOT NULL, + FOREIGN KEY (inbox_id) REFERENCES email_inboxes(id) ON DELETE CASCADE );`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/email/test-schema.ts` around lines 30 - 41, The CREATE TABLE for email_inbox_addresses is missing the foreign key on inbox_id; update the email_inbox_addresses table definition to add a foreign key constraint referencing email_inboxes(id) with ON DELETE CASCADE (e.g., add "FOREIGN KEY (inbox_id) REFERENCES email_inboxes(id) ON DELETE CASCADE") so tests mimic migration behavior and orphaned rows are prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/email/test-schema.ts`:
- Around line 30-41: The CREATE TABLE for email_inbox_addresses is missing the
foreign key on inbox_id; update the email_inbox_addresses table definition to
add a foreign key constraint referencing email_inboxes(id) with ON DELETE
CASCADE (e.g., add "FOREIGN KEY (inbox_id) REFERENCES email_inboxes(id) ON
DELETE CASCADE") so tests mimic migration behavior and orphaned rows are
prevented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 733f94f7-58ed-4cae-af9c-4af0c9d6ac14
📒 Files selected for processing (16)
packages/mock-servers/cloudflare/src/worker.tspackages/shared/src/outbound-email.tspackages/worker/migrations/0030-email-primitives.sqlpackages/worker/src/app/email/cloudflare-email.node.test.tspackages/worker/src/app/email/cloudflare-email.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/outbound.workers.test.tspackages/worker/src/email/parser.node.test.tspackages/worker/src/email/parser.tspackages/worker/src/email/repo.tspackages/worker/src/email/test-schema.tspackages/worker/src/mcp/capabilities/email/email-inbox-create.tspackages/worker/src/mcp/capabilities/email/email-reply.tspackages/worker/src/mcp/capabilities/email/email-send.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/email/parser.node.test.ts
- packages/worker/src/mcp/capabilities/email/email-inbox-create.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 5 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ddeae86. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Jul 6, 2026
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EMAILbinding, typed env support, and D1 email schema for inboxes, aliases, sender identities, sender policies, threads, messages, attachments, and delivery events.email()handling.emaildomain with inbox, message, sender policy, send, and reply capabilities; add package/runtime gating placeholders and setup/use documentation.Testing
npx vitest run --project workers-unit packages/worker/src/email/inbound.workers.test.ts packages/worker/src/email/outbound.workers.test.tsnpx vitest run --project node-unit packages/worker/src/email/address.node.test.ts packages/worker/src/mcp/capabilities/email/email-domain.node.test.tsnpx tsc -b --noEmit --pretty false && npm run test -- --run packages/worker/src/email/address.node.test.ts packages/worker/src/email/policy.node.test.ts packages/worker/src/email/parser.node.test.ts packages/worker/src/email/inbound.workers.test.ts packages/worker/src/email/outbound.workers.test.ts packages/worker/src/mcp/capabilities/email/email-domain.node.test.tsnpm run test && npm run typecheck && npm run buildNotes
Summary by CodeRabbit
New Features
Documentation