Repository navigation
feat(email): cut owner reads over to Mailbox - #1152
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>
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>
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>
Restore deleteEmailMessageById to D1 batch then immediate R2 cleanup with no Mailbox env/waitUntil/mirror. Restore insertEmailMessageWithAttachments signature without mirror forwarding. Drop PR-only delete mirror tests and update data-storage.md: live explicit/retention deletes are repaired by parity purge/rebuild; direct delete wiring remains pending. 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>
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>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR activates a default-off, two-hour parity-gated Mailbox read cutover. Account email and MCP reads use owner-scoped Mailbox APIs when enabled, while D1 remains the fallback and write authority. Feature-flag exposure recording is memoized and suppressible for SSR requests. ChangesMailbox read cutover
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AccountEmailOrMCP
participant MailboxReadCutover
participant FeatureFlagService
participant D1
participant MailboxDO
AccountEmailOrMCP->>MailboxReadCutover: request owner-scoped email data
MailboxReadCutover->>FeatureFlagService: evaluate flag and record exposure
MailboxReadCutover->>D1: verify parity and resolve user identity
MailboxReadCutover->>MailboxDO: read Mailbox data when gate passes
MailboxReadCutover->>D1: read D1 data when gate does not pass
MailboxReadCutover-->>AccountEmailOrMCP: return mapped email records
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-1152.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/worker/src/app/account-email-data.node.test.ts (1)
158-198: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard fake-timer cleanup with
afterEachorfinally.Both tests call
vi.useRealTimers()as their last statement. If anexpectcall earlier in the test fails,vi.useRealTimers()never runs, and the fake timers leak into later tests in the file, potentially causing unrelated failures.Use
afterEach(() => vi.useRealTimers())once for the file, or wrap each test body intry/finally.♻️ Proposed fix using afterEach
-import { expect, test, vi } from 'vitest' +import { afterEach, expect, test, vi } from 'vitest' ... +afterEach(() => { + vi.useRealTimers() +}) + test('loadAccountEmailData skips cutover exposure when SSR requests it', async () => { vi.useFakeTimers({ toFake: ['Date'] }) vi.setSystemTime(new Date('2026-07-31T15:00:00.000Z')) recordFeatureFlagExposuresMock.mockClear() await loadAccountEmailData({ ... }) expect(recordFeatureFlagExposuresMock).not.toHaveBeenCalled() - vi.useRealTimers() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/app/account-email-data.node.test.ts` around lines 158 - 198, Update the test cleanup for the fake timers used by the loadAccountEmailData tests by adding a file-level afterEach that calls vi.useRealTimers(). Remove the per-test cleanup calls from “skips cutover exposure when SSR requests it” and “records cutover exposure once for JSON API loads” so timer restoration also occurs when an assertion fails.packages/worker/src/email/mailbox-do.workers.test.ts (1)
122-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a non-matching filter assertion to
countMessages.Both count assertions expect
total: 1. A filter builder that drops thequeryandclassificationpredicates would still pass. Add one case that must return0.♻️ Suggested addition
expect(await mailboxA.countMessages({})).toEqual({ total: 1 }) expect( await mailboxA.countMessages({ query: 'project', classification: 'accepted', }), ).toEqual({ total: 1 }) + expect( + await mailboxA.countMessages({ query: 'no-such-subject' }), + ).toEqual({ total: 0 })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/mailbox-do.workers.test.ts` around lines 122 - 128, Add a countMessages assertion in the mailboxA test that uses a query or classification filter known not to match any message and expects total: 0, while preserving the existing matching-filter assertion to verify predicates are applied.docs/contributing/architecture/feature-flags.md (1)
85-89: 📐 Maintainability & Code Quality | 🔵 TrivialTrack the launch-hardening TODO outside the document.
The soak drops from 24 hours to 2 hours. The rationale and the fail-closed checks are documented, and the flag stays default-off, so the risk is contained for the pre-launch fleet. The
TODO(launch-hardening)marker has no tracked owner or trigger condition. It will be hard to find once the fleet grows.Do you want me to open an issue that records the revisit condition, for example a fleet-size threshold?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/contributing/architecture/feature-flags.md` around lines 85 - 89, Remove the untracked TODO(launch-hardening) marker from the mailbox_parity_matching_since documentation and create or reference a tracked issue capturing the need to revisit the 2-hour soak, including an owner and concrete trigger such as a fleet-size threshold. Preserve the existing rationale and fail-closed checks in the document.packages/worker/src/email/mailbox-read-cutover.node.test.ts (2)
538-549: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd page coverage for a non-empty query.
This case passes
query: ''. InlistOwnerEmailMessagesPage, an empty query resolves toquery: nullforcountMessagesand selects thelistMessagesbranch. ThesearchMessagesbranch and thecountMessagescall with a real query string stay uncovered.That combination is where
totaland the returned rows can disagree, because the count and the page build their text filter from different values. Add a case withquery: 'Hello'and assert bothtotaland thesearchMessagescall arguments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/mailbox-read-cutover.node.test.ts` around lines 538 - 549, Extend the mailbox-read cutover tests around listOwnerEmailMessagesPage with a non-empty query case using query: 'Hello'. Assert the result total and messages, and verify searchMessages receives the expected query arguments; ensure this case also exercises countMessages with the real query string rather than the empty-query listMessages branch.
655-690: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBoth cross-owner attachment cases run on the D1 path.
Lines 655-672 and 676-688 both pass
APP_DB: parityErrorDb. A parity read error fails closed, souseMailboxReadsreturnsfalsein both cases. Both assertions therefore exerciselistEmailAttachmentsForUserMessageon D1, which the assertions at lines 668 and 689 confirm.The test name states "isolates cross-owner identity", but no case reaches the Mailbox branch of
listOwnerEmailAttachmentsForMessage. Add a case that usesreadyDbwithstableUserId: 'user-other'and asserts the gate falls back to D1 rather than addressing another owner's Durable Object. The isolation guarantee for the Mailbox branch depends onmailboxRpcderiving the Durable Object name fromstableUserId; a direct assertion onidFromNamefor the rejected identity would pin that.As per coding guidelines: "Every signed-in user must have a fully isolated personal assistant, including separate packages, jobs, secrets, values, memories, remote connectors, email inboxes, and durable storage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/mailbox-read-cutover.node.test.ts` around lines 655 - 690, Add a cross-owner case in the test using readyDb and stableUserId 'user-other' so useMailboxReads permits the Mailbox path, then assert listOwnerEmailAttachmentsForMessage falls back to D1 without invoking another owner's Durable Object. Verify mailboxRpc derives its Durable Object name from the rejected stableUserId by asserting idFromName receives 'user-other', while preserving the existing D1 assertions.Source: Coding guidelines
packages/worker/src/email/mailbox-read-cutover.ts (1)
185-206: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winKey the memo to the account it was evaluated for.
MailboxReadCutoverMemoholds onegatePromiseand no owner identity.isMailboxReadCutoverEnabledreturns that promise without comparingdbUserIdorstableUserId. All current callers create one memo per authenticated user, so there is no live defect. If any future caller reuses a memo across users, one account's cutover decision applies to another account. Store the identity in the memo and re-evaluate on mismatch.As per coding guidelines: "Every signed-in user must have a fully isolated personal assistant, including separate packages, jobs, secrets, values, memories, remote connectors, email inboxes, and durable storage."
🛡️ Proposed fix to bind the memo to one account
export type MailboxReadCutoverMemo = { + gateKey?: string gatePromise?: Promise<boolean> }memo?: MailboxReadCutoverMemo }): Promise<boolean> { - if (input.memo?.gatePromise) return input.memo.gatePromise + const gateKey = `${input.dbUserId}:${input.stableUserId}` + if (input.memo?.gatePromise && input.memo.gateKey === gateKey) { + return input.memo.gatePromise + } const promise = evaluateCutoverGate(input) - if (input.memo) input.memo.gatePromise = promise + if (input.memo) { + input.memo.gateKey = gateKey + input.memo.gatePromise = promise + } return promise }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/mailbox-read-cutover.ts` around lines 185 - 206, Update MailboxReadCutoverMemo and isMailboxReadCutoverEnabled to bind the cached gatePromise to both dbUserId and stableUserId. Reuse the promise only when both identities match; otherwise clear or replace the memo’s stored identity and gatePromise before calling evaluateCutoverGate, ensuring decisions cannot be shared across accounts.Source: Coding guidelines
packages/worker/src/email/service.ts (1)
620-643: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider fetching the message only when raw MIME is needed.
loadEmailAttachmentContentignoresmessagewhenstorageKindisexternalorunavailable.getEmailAttachmentByIdalways runs the extragetEmailMessageByIdquery. The same pattern exists ingetOwnerEmailAttachmentByIdinpackages/worker/src/email/mailbox-read-cutover.ts, where the extra read can become a Durable Object RPC. Passing a lazy message loader would remove that read for externally stored attachments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/email/service.ts` around lines 620 - 643, Update getEmailAttachmentById and getOwnerEmailAttachmentById to pass a lazy message loader to loadEmailAttachmentContent instead of eagerly calling getEmailMessageById. Have loadEmailAttachmentContent invoke the loader only for storage kinds that require raw MIME, while preserving existing behavior for external and unavailable attachments.packages/worker/src/mcp/capabilities/email/email-message-get.ts (1)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "resolve dbUserId or throw" block into a shared helper.
The same five-line block — call
resolveMailboxReadCutoverDbUserId, check fornull, throwError('Authenticated user could not be resolved.')— is copy-pasted across all five email MCP capability handlers. Consolidate this into one helper, for examplerequireOwnerDbUserId(env, stableUserId)placed alongsiderequireVerifiedEmailAccountUser, so future changes to the resolution or error-handling logic only need to happen in one place.
packages/worker/src/mcp/capabilities/email/email-message-get.ts#L29-L35: replace this block with a call to the new shared helper.packages/worker/src/mcp/capabilities/email/email-attachment-get.ts#L37-L43: replace this block with a call to the new shared helper.packages/worker/src/mcp/capabilities/email/email-delivery-event-list.ts#L63-L69: replace this block with a call to the new shared helper.packages/worker/src/mcp/capabilities/email/email-message-list.ts#L38-L44: replace this block with a call to the new shared helper.packages/worker/src/mcp/capabilities/email/email-message-search.ts#L46-L52: replace this block with a call to the new shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/capabilities/email/email-message-get.ts` around lines 29 - 35, Extract the repeated resolve-or-throw logic into a shared requireOwnerDbUserId helper alongside requireVerifiedEmailAccountUser, preserving the existing resolver call and error message. Replace the duplicated blocks in packages/worker/src/mcp/capabilities/email/email-message-get.ts:29-35, packages/worker/src/mcp/capabilities/email/email-attachment-get.ts:37-43, packages/worker/src/mcp/capabilities/email/email-delivery-event-list.ts:63-69, packages/worker/src/mcp/capabilities/email/email-message-list.ts:38-44, and packages/worker/src/mcp/capabilities/email/email-message-search.ts:46-52 with calls to the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/email/mailbox-read-cutover.ts`:
- Around line 514-531: Update getOwnerEmailAttachmentById to create a shared
memo when input.memo is absent, then pass that memo to both
getOwnerEmailAttachmentRecordById and getOwnerEmailMessageById so
useMailboxReads evaluates the gate and records exposure only once per attachment
fetch.
- Around line 371-385: Update the D1 fallback in searchOwnerEmailMessages to
preserve the classification filter accepted by the function and applied by the
Mailbox path. Apply the filter to the D1 search results before returning them,
or remove classification from the function signature if D1 cannot support it;
keep both gate branches behaviorally equivalent.
In `@packages/worker/src/email/mailbox-store.ts`:
- Around line 74-82: Align query normalization between count and page paths: in
buildMailboxMessageFilterClauses, trim input.query first, skip the text clause
when the trimmed value is empty, and wrap subject and from_address with COALESCE
so NULL values remain searchable. In
packages/worker/src/email/mailbox-read-cutover.node.test.ts lines 538-549, add a
listOwnerEmailMessagesPage case with a non-empty query asserting total equals
returned rows and verifying the countMessages and searchMessages arguments.
---
Nitpick comments:
In `@docs/contributing/architecture/feature-flags.md`:
- Around line 85-89: Remove the untracked TODO(launch-hardening) marker from the
mailbox_parity_matching_since documentation and create or reference a tracked
issue capturing the need to revisit the 2-hour soak, including an owner and
concrete trigger such as a fleet-size threshold. Preserve the existing rationale
and fail-closed checks in the document.
In `@packages/worker/src/app/account-email-data.node.test.ts`:
- Around line 158-198: Update the test cleanup for the fake timers used by the
loadAccountEmailData tests by adding a file-level afterEach that calls
vi.useRealTimers(). Remove the per-test cleanup calls from “skips cutover
exposure when SSR requests it” and “records cutover exposure once for JSON API
loads” so timer restoration also occurs when an assertion fails.
In `@packages/worker/src/email/mailbox-do.workers.test.ts`:
- Around line 122-128: Add a countMessages assertion in the mailboxA test that
uses a query or classification filter known not to match any message and expects
total: 0, while preserving the existing matching-filter assertion to verify
predicates are applied.
In `@packages/worker/src/email/mailbox-read-cutover.node.test.ts`:
- Around line 538-549: Extend the mailbox-read cutover tests around
listOwnerEmailMessagesPage with a non-empty query case using query: 'Hello'.
Assert the result total and messages, and verify searchMessages receives the
expected query arguments; ensure this case also exercises countMessages with the
real query string rather than the empty-query listMessages branch.
- Around line 655-690: Add a cross-owner case in the test using readyDb and
stableUserId 'user-other' so useMailboxReads permits the Mailbox path, then
assert listOwnerEmailAttachmentsForMessage falls back to D1 without invoking
another owner's Durable Object. Verify mailboxRpc derives its Durable Object
name from the rejected stableUserId by asserting idFromName receives
'user-other', while preserving the existing D1 assertions.
In `@packages/worker/src/email/mailbox-read-cutover.ts`:
- Around line 185-206: Update MailboxReadCutoverMemo and
isMailboxReadCutoverEnabled to bind the cached gatePromise to both dbUserId and
stableUserId. Reuse the promise only when both identities match; otherwise clear
or replace the memo’s stored identity and gatePromise before calling
evaluateCutoverGate, ensuring decisions cannot be shared across accounts.
In `@packages/worker/src/email/service.ts`:
- Around line 620-643: Update getEmailAttachmentById and
getOwnerEmailAttachmentById to pass a lazy message loader to
loadEmailAttachmentContent instead of eagerly calling getEmailMessageById. Have
loadEmailAttachmentContent invoke the loader only for storage kinds that require
raw MIME, while preserving existing behavior for external and unavailable
attachments.
In `@packages/worker/src/mcp/capabilities/email/email-message-get.ts`:
- Around line 29-35: Extract the repeated resolve-or-throw logic into a shared
requireOwnerDbUserId helper alongside requireVerifiedEmailAccountUser,
preserving the existing resolver call and error message. Replace the duplicated
blocks in packages/worker/src/mcp/capabilities/email/email-message-get.ts:29-35,
packages/worker/src/mcp/capabilities/email/email-attachment-get.ts:37-43,
packages/worker/src/mcp/capabilities/email/email-delivery-event-list.ts:63-69,
packages/worker/src/mcp/capabilities/email/email-message-list.ts:38-44, and
packages/worker/src/mcp/capabilities/email/email-message-search.ts:46-52 with
calls to the helper.
🪄 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: 5650954e-ad0f-4786-a345-0a9b41942fe7
📒 Files selected for processing (28)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mdpackages/worker/src/app/account-email-data.node.test.tspackages/worker/src/app/account-email-data.tspackages/worker/src/app/handlers/account-email-exposure.node.test.tspackages/worker/src/app/handlers/account-email.node.test.tspackages/worker/src/app/handlers/account-email.tspackages/worker/src/app/request-feature-flags-cache.tspackages/worker/src/email/mailbox-do.tspackages/worker/src/email/mailbox-do.workers.test.tspackages/worker/src/email/mailbox-read-cutover.node.test.tspackages/worker/src/email/mailbox-read-cutover.tspackages/worker/src/email/mailbox-store.tspackages/worker/src/email/mailbox-types.tspackages/worker/src/email/repo-attachments.workers.test.tspackages/worker/src/email/repo.tspackages/worker/src/email/service.tspackages/worker/src/feature-flags/registry.tspackages/worker/src/feature-flags/service.tspackages/worker/src/mcp/capabilities/email/email-attachment-get.node.test.tspackages/worker/src/mcp/capabilities/email/email-attachment-get.tspackages/worker/src/mcp/capabilities/email/email-delivery-event-list.node.test.tspackages/worker/src/mcp/capabilities/email/email-delivery-event-list.tspackages/worker/src/mcp/capabilities/email/email-message-get.tspackages/worker/src/mcp/capabilities/email/email-message-list.node.test.tspackages/worker/src/mcp/capabilities/email/email-message-list.tspackages/worker/src/mcp/capabilities/email/email-message-search.node.test.tspackages/worker/src/mcp/capabilities/email/email-message-search.ts
| const useMailbox = await useMailboxReads(input) | ||
| if (!useMailbox) { | ||
| // D1 search has no classification filter; callers that need it use | ||
| // listOwnerEmailMessagesPage (account inbox) or list with filters. | ||
| return searchEmailMessages({ | ||
| db: input.env.APP_DB, | ||
| userId: input.stableUserId, | ||
| query: input.query, | ||
| inboxId: input.inboxId, | ||
| direction: input.direction, | ||
| processingStatus: input.processingStatus, | ||
| deliveryStatus: input.deliveryStatus, | ||
| limit: input.limit, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the classification filter on the D1 search branch.
searchOwnerEmailMessages accepts classification and passes it to the Mailbox branch. The D1 branch drops it. The same call then returns different result sets depending on the gate state, which contradicts the parity premise of the cutover. No current caller passes classification, so there is no live divergence. Filter the D1 results in memory, or remove classification from the signature until D1 search supports it.
🐛 Proposed fix to keep both branches equivalent
const useMailbox = await useMailboxReads(input)
if (!useMailbox) {
- // D1 search has no classification filter; callers that need it use
- // listOwnerEmailMessagesPage (account inbox) or list with filters.
- return searchEmailMessages({
+ // D1 search has no classification predicate, so filter after the query.
+ const found = await searchEmailMessages({
db: input.env.APP_DB,
userId: input.stableUserId,
query: input.query,
inboxId: input.inboxId,
direction: input.direction,
processingStatus: input.processingStatus,
deliveryStatus: input.deliveryStatus,
limit: input.limit,
})
+ if (!input.classification) return found
+ return found.filter(
+ (message) => message.classification === input.classification,
+ )
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/email/mailbox-read-cutover.ts` around lines 371 - 385,
Update the D1 fallback in searchOwnerEmailMessages to preserve the
classification filter accepted by the function and applied by the Mailbox path.
Apply the filter to the D1 search results before returning them, or remove
classification from the function signature if D1 cannot support it; keep both
gate branches behaviorally equivalent.
| export async function getOwnerEmailAttachmentById( | ||
| input: OwnerReadBase & { | ||
| blobs: R2Bucket | ||
| attachmentId: string | ||
| }, | ||
| ) { | ||
| const attachment = await getOwnerEmailAttachmentRecordById(input) | ||
| if (!attachment) return null | ||
| const message = await getOwnerEmailMessageById({ | ||
| ...input, | ||
| messageId: attachment.messageId, | ||
| }) | ||
| return loadEmailAttachmentContent({ | ||
| blobs: input.blobs, | ||
| attachment, | ||
| message, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Create a memo when the caller does not supply one.
getOwnerEmailAttachmentById calls getOwnerEmailAttachmentRecordById and then getOwnerEmailMessageById. Each call runs useMailboxReads independently. If input.memo is absent, the gate evaluates twice, which runs the flag and parity queries twice and records the mailbox-read-cutover exposure twice for one operation. packages/worker/src/mcp/capabilities/email/email-attachment-get.ts calls this function without a memo, so every MCP attachment fetch double-counts the exposure and skews the success metric. Default the memo inside this function.
🐛 Proposed fix to evaluate the gate once per attachment fetch
) {
- const attachment = await getOwnerEmailAttachmentRecordById(input)
+ const memo = input.memo ?? {}
+ const attachment = await getOwnerEmailAttachmentRecordById({ ...input, memo })
if (!attachment) return null
const message = await getOwnerEmailMessageById({
...input,
+ memo,
messageId: attachment.messageId,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getOwnerEmailAttachmentById( | |
| input: OwnerReadBase & { | |
| blobs: R2Bucket | |
| attachmentId: string | |
| }, | |
| ) { | |
| const attachment = await getOwnerEmailAttachmentRecordById(input) | |
| if (!attachment) return null | |
| const message = await getOwnerEmailMessageById({ | |
| ...input, | |
| messageId: attachment.messageId, | |
| }) | |
| return loadEmailAttachmentContent({ | |
| blobs: input.blobs, | |
| attachment, | |
| message, | |
| }) | |
| } | |
| export async function getOwnerEmailAttachmentById( | |
| input: OwnerReadBase & { | |
| blobs: R2Bucket | |
| attachmentId: string | |
| }, | |
| ) { | |
| const memo = input.memo ?? {} | |
| const attachment = await getOwnerEmailAttachmentRecordById({ ...input, memo }) | |
| if (!attachment) return null | |
| const message = await getOwnerEmailMessageById({ | |
| ...input, | |
| memo, | |
| messageId: attachment.messageId, | |
| }) | |
| return loadEmailAttachmentContent({ | |
| blobs: input.blobs, | |
| attachment, | |
| message, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/email/mailbox-read-cutover.ts` around lines 514 - 531,
Update getOwnerEmailAttachmentById to create a shared memo when input.memo is
absent, then pass that memo to both getOwnerEmailAttachmentRecordById and
getOwnerEmailMessageById so useMailboxReads evaluates the gate and records
exposure only once per attachment fetch.
| if (typeof input.query === 'string') { | ||
| const pattern = `%${escapeLikePattern(input.query.trim().toLowerCase())}%` | ||
| clauses.push(`( | ||
| LOWER(subject) LIKE ? ESCAPE '\\' | ||
| OR LOWER(from_address) LIKE ? ESCAPE '\\' | ||
| OR LOWER(COALESCE(envelope_from, '')) LIKE ? ESCAPE '\\' | ||
| )`) | ||
| params.push(pattern, pattern, pattern) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The text filter is built from different values on the count path and the page path. buildMailboxMessageFilterClauses treats any string, including '', as an active filter, while listOwnerEmailMessagesPage normalizes an empty query to null before calling countMessages and passes the raw string to searchMessages. For an empty or whitespace-only query, the count applies no text filter and the page applies a %% filter that drops rows with NULL subject and NULL from_address, so total and the returned rows disagree.
packages/worker/src/email/mailbox-store.ts#L74-L82: trim the query first and skip the clause when the result is empty; wrapsubjectandfrom_addressinCOALESCE(..., '')soNULLcolumns do not drop rows.packages/worker/src/email/mailbox-read-cutover.node.test.ts#L538-L549: add alistOwnerEmailMessagesPagecase with a non-empty query that assertstotalmatches the returned row count and asserts thecountMessagesandsearchMessagescall arguments.
📍 Affects 2 files
packages/worker/src/email/mailbox-store.ts#L74-L82(this comment)packages/worker/src/email/mailbox-read-cutover.node.test.ts#L538-L549
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/email/mailbox-store.ts` around lines 74 - 82, Align query
normalization between count and page paths: in buildMailboxMessageFilterClauses,
trim input.query first, skip the text clause when the trimmed value is empty,
and wrap subject and from_address with COALESCE so NULL values remain
searchable. In packages/worker/src/email/mailbox-read-cutover.node.test.ts lines
538-549, add a listOwnerEmailMessagesPage case with a non-empty query asserting
total equals returned rows and verifying the countMessages and searchMessages
arguments.
Summary
Owner-facing account/MCP email reads are live on Mailbox. D1 dual-writes remain enabled.
Backup and production verification
kody-email-blobs/migration-backups/mailbox-read-cutover/2026-08-02T02-19Z/user-email-metadata-v1.json7787f8c9af94a483432b89ae91279b2d8c9564056da18bcdade34be5b711f710Contract-chain technical stop
D1 write-off/table retirement is blocked by live authorities still sharing the four tables:
provider_message_id → user/messagelookup inemail_messages.system:emailintentionally remains in D1 and uses the same message/thread/attachment/event tables, so the tables cannot be dropped without first splitting an operator-only graph.Stopping user writes or dropping tables before these are migrated would break retry idempotency, provider delivery updates, system mail, and account portability. No destructive contract migration was created.
Conductor report
Summary by CodeRabbit