Repository navigation
feat(email): move system email authority to dedicated graph - #1163
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe PR makes dedicated system-email graph tables authoritative. It adds migration and parity gates, transactional legacy mirrors, dedicated inbound delivery and effect processing, dedicated reads and retention, and updated administration, usage, provider-index, and documentation paths. ChangesDedicated system-email authority
Estimated code review effort: 5 (Critical) | ~120 minutes 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>
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>
|
🔎 Preview deployed: https://kody-pr-1163.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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/worker/src/email/service.ts (1)
473-498: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the Mailbox authority check outside the
tryblock.Line 484 throws
'User inbound finalization requires Mailbox authority.'inside thetry. Thecatchat Line 487 then evaluatesinput.authority?.get(...), which resolves toundefinedwhenauthorityis missing.committed?.state !== 'received'is therefore true, and the permanent configuration error is rewrapped as aRetryableInboundStorageError. The delivery is then retried indefinitely, because a missing authority is not transient.Validate
input.authoritybefore thetryso the error propagates as non-retryable.🐛 Proposed fix
let finalizedDelivery: InboundDelivery + const authority = input.authority + if (!authority) { + throw new Error('User inbound finalization requires Mailbox authority.') + } try { const finalization = { delivery, usageDurationMs: delivery.usageStartedAt ? Date.now() - Date.parse(delivery.usageStartedAt) : 0, usageMonth: (stored.receivedAt ?? stored.createdAt).slice(0, 7), usageBytes: stored.rawSize ?? 0, } - if (!input.authority) { - throw new Error('User inbound finalization requires Mailbox authority.') - } - finalizedDelivery = await input.authority.receive(finalization) + finalizedDelivery = await authority.receive(finalization) } catch (error) { - const committed = await input.authority - ?.get(delivery.deliveryId) - .catch(() => null) + const committed = await authority.get(delivery.deliveryId).catch(() => null) if (committed?.state !== 'received') {🤖 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 473 - 498, Move the input.authority validation and its existing “User inbound finalization requires Mailbox authority.” error before the try block surrounding finalization. Keep the receive call and retryable failure handling inside the try, so a missing authority propagates directly instead of being wrapped as a RetryableInboundStorageError.packages/worker/src/email/inbound.ts (1)
74-92: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake
authorityrequired instead of optional.
rejectClaimedInboundDeliverythrows wheninput.authorityis missing. System inbound rejection now goes entirely throughhandleSystemInboundEmail, so this function has only one caller, and that caller always suppliesauthority. Change the field from optional to required so a future caller omission surfaces at compile time instead of at runtime.✏️ Proposed type tightening
async function rejectClaimedInboundDelivery(input: { db: D1Database message: ForwardableEmailMessage delivery: InboundDelivery reason: string - authority?: UserInboundDeliveryAuthority + authority: UserInboundDeliveryAuthority }) { - if (!input.authority) { - throw new Error('User inbound rejection requires Mailbox authority.') - } const transitioned = await input.authority .reject(input.delivery, input.reason) .catch((error: unknown) => { warnRejectionAuditWriteFailed(error) throw error }) if (transitioned) input.message.setReject(input.reason) return transitioned }🤖 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/inbound.ts` around lines 74 - 92, Update the input type of rejectClaimedInboundDelivery so authority is required rather than optional, and remove the now-unnecessary missing-authority runtime check while preserving the existing rejection and audit-error flow.
🧹 Nitpick comments (15)
packages/worker/src/email/system-email-retention-graph.workers.test.ts (1)
74-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the row count of the recursive CTE explicit.
VALUES(0) UNION ALL SELECT value + 1 FROM sequence WHERE value < ?bound tosystemEmailLimits.maxStoredMessagesproducesmaxStoredMessages + 1rows, notmaxStoredMessages. The expectations at Lines 116, 134, and 135 depend on that inclusive bound. Add a comment stating the intended count, so a later edit to the bound does not silently break the arithmetic.♻️ Proposed comment
+ // Seeds maxStoredMessages + 1 fresh rows. With the single old message this + // puts the graph one row over the cap after the age prune, so the cap prune + // deletes exactly one more message. await env.APP_DB.prepare( `WITH RECURSIVE sequence(value) AS (🤖 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/system-email-retention-graph.workers.test.ts` around lines 74 - 88, Add a concise comment immediately above the recursive CTE in the setup query explaining that the seed value plus the inclusive recursive bound intentionally generates systemEmailLimits.maxStoredMessages + 1 rows, matching the expectations later in the test. Leave the query and its binding unchanged.packages/worker/src/email/system-email-graph-repo.ts (2)
237-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 242 is redundant.
Lines 238 and 240 already require
report.linkedMessageCount === 0anddedicatedProviderLinkedMessageCount === 0. The equality check on Line 242 can never change the result. Remove it, or keep it and add a comment that it is intentional defensive duplication.♻️ Proposed simplification
const providerParity = report.linkedMessageCount === 0 && report.indexCount === 0 && dedicatedProviderLinkedMessageCount === 0 && - report.parity && - report.linkedMessageCount === dedicatedProviderLinkedMessageCount + report.parity🤖 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/system-email-graph-repo.ts` around lines 237 - 245, Remove the redundant report.linkedMessageCount === dedicatedProviderLinkedMessageCount condition from the providerParity calculation; the existing zero-count checks already establish this equality. Keep the remaining providerParity conditions and classification behavior unchanged.
338-373: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMirror ordering and result indexing are correct; note the double authority read.
childFirstreverses the contracts to[deliveryEvents, attachments, messages, threads], which matches the index mapping indeletedMutationCounts. The upsert offset of 4 matches the four delete statements. The in-batchsystemEmailAuthorityGuardStatementcloses the TOCTOU window left by the pre-batchassertSystemEmailGraphAuthoritycall, because the guard violates thesingleton = 1CHECK and rolls the batch back.One note:
assertSystemEmailGraphAuthorityruns at Line 348 and again insideloadSystemEmailGraphParityReportat Line 364. That is two extra marker reads per reconcile. The cost is small, but you can pass a flag to skip the second assertion if this path becomes hot.🤖 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/system-email-graph-repo.ts` around lines 338 - 373, The reconcile flow performs the authority assertion twice: once in reconcileLegacySystemEmailGraphFromDedicated and again through loadSystemEmailGraphParityReport. If this path is optimized, add an explicit option to loadSystemEmailGraphParityReport to skip its authority assertion and use that option here, while preserving the default assertion behavior for other callers.packages/worker/migrations/0131-system-email-graph-authority.sql (1)
873-897: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
ON CONFLICT(singleton) DO UPDATEis unreachable and can hide a re-run failure.
singletonhasCHECK (singleton = 1)and the SELECT always produces1. A second run therefore conflicts and updates the row. If the second run computes a non-zero count, the UPDATE violates the column CHECK and the migration fails, which is correct. But if the marker already exists from a partial earlier apply, the UPDATE silently refreshescutover_atand hides that a re-apply occurred. Consider keepingDO NOTHINGplus an explicit re-verification, or record an apply counter.🤖 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/migrations/0131-system-email-graph-authority.sql` around lines 873 - 897, The migration’s ON CONFLICT update can silently overwrite an existing authority marker during re-runs. In the singleton insert, replace the DO UPDATE behavior with DO NOTHING and add explicit re-verification that detects and fails on any non-zero graph mismatch or provider-link count, while preserving the existing initial count calculation.packages/worker/src/email/system-email-graph-sql.ts (1)
143-154: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
legacyColumnsandselectedboth assumecolumns[0]isid.
contract.columns.slice(0, 1)andcontract.columns.slice(1)encode a positional assumption. If a contract insystemEmailGraphColumnContractsever listsidin another position, the generated SQL binds?1to the wrong column and writes the owner id into a data column. Add an assertion.🛡️ Proposed guard
+ if (contract.columns[0] !== 'id') { + throw new Error( + `System email graph contract ${contract.key} must list id first.`, + ) + } const legacyColumns = [ ...contract.columns.slice(0, 1), 'user_id', ...contract.columns.slice(1), ].join(', ')🤖 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/system-email-graph-sql.ts` around lines 143 - 154, In the SQL generation function containing legacyColumns and selected, assert that contract.columns[0] is exactly 'id' before constructing the INSERT statement. Keep the existing positional column generation unchanged when the assertion passes, and fail immediately with a clear invariant error when a contract violates this ordering.packages/worker/src/email/system-email-graph-migration.node.test.ts (1)
192-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport the owner id instead of repeating the literal.
graphParityDifferenceCounthardcodes'system:email'. The module./email-owner.tsexportssystemEmailOwnerId, which the production code uses. If the constant ever changes, this helper compares the wrong rows and reports parity for an empty set. Bind the imported constant instead.🤖 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/system-email-graph-migration.node.test.ts` around lines 192 - 221, Update graphParityDifferenceCount to import and bind systemEmailOwnerId from ./email-owner.ts instead of embedding the 'system:email' literal in its SQL queries, while preserving the existing legacy and dedicated comparison behavior.packages/worker/src/email/system-email-authority.ts (1)
34-60: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the authority check per request.
assertSystemEmailGraphAuthorityruns two extra round trips on every dedicated read and write. Several call paths run it repeatedly for one logical operation. For example,deleteSystemEmailMessageByIdinpackages/worker/src/email/system-email-graph-store.tstriggers it throughgetSystemEmailMessageByIdandlistSystemEmailAttachmentsbefore the mutation. The second query also scansemail_messagesandsystem_email_messagesfor provider links. A per-invocation memo (for example aWeakMapkeyed by theD1Databasebinding, reset per request) keeps the fail-closed behavior and removes the repeated cost.🤖 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/system-email-authority.ts` around lines 34 - 60, Cache the successful authority result used by assertSystemEmailGraphAuthority in a per-request memo keyed by the D1Database binding, so repeated calls during one logical operation skip both queries. Ensure the memo is reset for each request and that failed or invalid checks are not cached, preserving the existing fail-closed behavior.packages/worker/src/email/system-email-graph-store.ts (1)
330-336: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRemove the positional assumption about the first two message columns.
legacyColumnsandlegacyValuesassume thatsystemEmailMessageColumns[0]isidand[1]isdirection. If the contract array is reordered, the insert still compiles and still binds the correct arity, but it writes values into the wrong legacy columns. The mirror guard compares the same columns, so the drift can pass the parity check. Build the legacy lists from the column names instead of from slice offsets.♻️ Proposed refactor
- const legacyColumns = [...columns.slice(0, 2), 'user_id', ...columns.slice(2)] - const legacyValues = [ - row.id, - row.direction, - systemEmailOwnerId, - ...values.slice(2), - ] + const legacyColumns = [...columns, 'user_id'] + const legacyValues = [...values, systemEmailOwnerId]🤖 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/system-email-graph-store.ts` around lines 330 - 336, Update the legacy column/value construction near legacyColumns and legacyValues to locate the id and direction entries by their column names rather than assuming positions 0 and 1. Insert user_id at the corresponding legacy location and derive each value from the matching source column, preserving correct bindings if systemEmailMessageColumns is reordered.packages/worker/src/email/system-email-graph-transaction.ts (1)
96-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the result offsets explicit for callers.
Callers read mutation results by position, for example
results[index * 3]indeleteEmptySystemEmailThreadsandpruneSystemExpiredInboundDedupePointers. That arithmetic silently breaks if a caller passesbeforestatements or if the statements per mutation change. Return the mutation results in a named shape, or export the per-mutation statement count, so the offset stays correct.🤖 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/system-email-graph-transaction.ts` around lines 96 - 111, Update commitSystemEmailGraphMutations and its callers, including deleteEmptySystemEmailThreads and pruneSystemExpiredInboundDedupePointers, to expose or consume explicit mutation-result offsets instead of calculating positions as index * 3. Ensure offsets account for any before statements and remain correct if composeSystemEmailGraphMutation changes its per-mutation statement count; prefer returning mutation results in a named structure or reusing an exported statement-count constant.packages/worker/src/email/system-inbound-delivery-mirror.ts (1)
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
message_idternary is a no-op.Both branches produce the same text.
column === 'message_id'yieldsmessage_id = excluded.message_id, and the generic branch yields the same string. The special case suggests an intent that the code does not implement.♻️ Proposed simplification
${updateColumns - .map((column) => - column === 'message_id' - ? 'message_id = excluded.message_id' - : `${column} = excluded.${column}`, - ) + .map((column) => `${column} = excluded.${column}`) .join(',\n\t\t\t\t')},🤖 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/system-inbound-delivery-mirror.ts` around lines 50 - 56, Remove the redundant `column === 'message_id'` special case in the `updateColumns` mapping and use the generic `${column} = excluded.${column}` expression for every column, preserving the existing joined SQL output.packages/worker/src/email/system-inbound-effect-store.ts (1)
325-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind
systemEmailOwnerIdinstead of interpolating it.Line 326 interpolates the constant into the SQL text. Every other value in this file is bound. The constant is a module-level literal today, so there is no injection surface, but the mixed pattern forces a reader to verify the provenance of the value.
♻️ Proposed change
`SELECT - '${systemEmailOwnerId}' AS user_id, + ? AS user_id, 'email_received' AS metric,- .bind(systemInboundProvider, ...input.months) + .bind(systemEmailOwnerId, systemInboundProvider, ...input.months)🤖 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/system-inbound-effect-store.ts` around lines 325 - 340, Update the SQL query in the system inbound usage aggregation to bind systemEmailOwnerId as a parameter instead of interpolating it into the SELECT clause, and pass it through the existing bind call alongside systemInboundProvider and input.months. Preserve the query’s result and parameter ordering.packages/worker/src/email/reconcile-inbound-deliveries.ts (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
authority_*CASE branches are now unreachable.The
projected_eventsCTE excludes the system owner. EveryCASE WHEN user_id = system_owner_idexpression at Lines 56-107 therefore always resolves to itsELSEcolumn. Theauthority(system_owner_id)CTE and the first bound parameter exist only to feed that dead branch.Collapse the CASE expressions to plain column references when this query is next touched. That removes about fifty lines and one join from a query that runs on every sweep.
🤖 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/reconcile-inbound-deliveries.ts` at line 110, Update the reconciliation query around the projected_events CTE and its authority_* CASE expressions: since user_id excludes system_owner_id, replace each CASE with its ELSE column reference, then remove the now-unused authority(system_owner_id) CTE, related join, and first bound parameter. Preserve the remaining query semantics and parameter ordering.packages/worker/src/email/inbound-effects.ts (1)
743-769: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the error-message string match with an explicit capability check.
The fallback detects a missing
usage_rollupstable by matching the textno such table: usage_rollupsin the error message. This couples production code to a D1 error string. If the driver changes the wording, the fallback stops working and the whole effect fails.The intended condition is already expressed by
includeRollup: !input.env.USAGE_EVENTS. Resolve rollup availability once, then pass a stable flag.♻️ Sketch of a capability-based check
- try { - await recordSystemInboundUsageEffect({ - db: input.env.APP_DB, - delivery, - usageMonth, - usageBytes, - usageDurationMs, - now, - includeRollup: !input.env.USAGE_EVENTS, - }) - } catch (error) { - if ( - !(error instanceof Error) || - !error.message.includes('no such table: usage_rollups') - ) { - throw error - } - await recordSystemInboundUsageEffect({ - db: input.env.APP_DB, - delivery, - usageMonth, - usageBytes, - usageDurationMs, - now, - includeRollup: false, - }) - } + const includeRollup = + !input.env.USAGE_EVENTS && (await usageRollupsTableExists(input.env.APP_DB)) + await recordSystemInboundUsageEffect({ + db: input.env.APP_DB, + delivery, + usageMonth, + usageBytes, + usageDurationMs, + now, + includeRollup, + })🤖 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/inbound-effects.ts` around lines 743 - 769, Replace the error-message matching and retry in the inbound usage recording flow with an explicit rollup capability derived once from input.env.USAGE_EVENTS. Use that stable flag to set includeRollup when calling recordSystemInboundUsageEffect, removing the catch-based missing-table fallback while preserving rollup-disabled behavior when usage events are enabled.packages/worker/src/email/inbound-delivery.ts (1)
340-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the rejection throw outside the
tryblock.The cutover rejection is thrown inside the
trythat also guards the marker query. Thecatchthen inspects the error message to decide whether to swallow it. Today the rejection text does not containno such table: system_email_graph_authority, so the guard works. The structure still couples control flow to message matching. Read the marker in thetry, then decide after it.♻️ Proposed restructure
- try { - const marker = await db - .prepare( - `SELECT authority FROM system_email_graph_authority - WHERE singleton = 1`, - ) - .first<{ authority: string }>() - if (marker?.authority === 'dedicated') { - throw new Error( - 'Legacy system inbound D1 engine is compatibility-only after cutover.', - ) - } - } catch (error) { - if ( - error instanceof Error && - error.message.includes('no such table: system_email_graph_authority') - ) { - return - } - throw error - } + let marker: { authority: string } | null = null + try { + marker = await db + .prepare( + `SELECT authority FROM system_email_graph_authority + WHERE singleton = 1`, + ) + .first<{ authority: string }>() + } catch (error) { + if ( + error instanceof Error && + error.message.includes('no such table: system_email_graph_authority') + ) { + return + } + throw error + } + if (marker?.authority === 'dedicated') { + throw new Error( + 'Legacy system inbound D1 engine is compatibility-only after cutover.', + ) + }🤖 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/inbound-delivery.ts` around lines 340 - 360, Move the `marker?.authority === 'dedicated'` rejection decision out of the `try`/`catch`: keep only the marker query and missing-table handling inside the `try`, then check the returned marker and throw the compatibility error afterward. Preserve the existing return behavior when `system_email_graph_authority` is absent.packages/worker/src/email/system-inbound-transition-parity.node.test.ts (1)
59-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
dbfield fromcreateDatabase.
createDatabasebuilds a D1 wrapper, but the test discards it and builds a second wrapper from thesqlitefield. Return onlysqlite, or destructure both values, so each transition run uses one backing wrapper.
usingwithDatabaseSyncfromnode:sqliteis supported with the declared Node 26+ runtime.🤖 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/system-inbound-transition-parity.node.test.ts` at line 59, Update createDatabase to return only the sqlite instance and remove the unused db field; ensure transition setup uses that single backing wrapper without creating a second D1 wrapper. Preserve the existing using-based DatabaseSync lifecycle.
🤖 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 `@docs/contributing/architecture/data-storage.md`:
- Around line 826-831: In the documentation describing the audited
system_email_graph_reconcile action, change the phrase “upserts the complete
dedicated graph parent-first” to “upserts the complete legacy graph
parent-first,” preserving the surrounding rollback-mirror behavior and direction
requirements.
In `@packages/worker/migrations/0131-system-email-graph-authority.sql`:
- Around line 441-451: Make the migration’s graph table creation and
authority-marker write execute as one atomic migration unit, ensuring any CHECK
failure rolls back both DDL and data changes. Update the migration structure or
execution mechanism around the graph-table creation and the INSERT into
system_email_graph_authority so partial graph state cannot remain when the
marker write fails.
In `@packages/worker/src/email/inbound-classification.ts`:
- Around line 24-31: Remove the special-case catch in the email classification
flow that suppresses errors containing “no such table: email_sender_rules”; let
that database error propagate like other failures. Keep the test schema’s
existing email_sender_rules setup unchanged.
In `@packages/worker/src/email/reconcile-inbound-deliveries.ts`:
- Line 32: Remove the top-level assertSystemEmailGraphAuthority call from the
reconciliation flow and move it into reconcileUser, guarded by userId ===
systemEmailOwnerId. Keep the guard inside the existing per-owner try/catch so
system-owner failures are recorded without preventing ordinary users from
completing reconciliation.
- Around line 195-221: Align the systemDue EXISTS predicate with
listDueSystemInboundEffects by adding the missing stale-event
state/cleanup-retry eligibility filter and effect retry-time/lease-expiry
checks, reusing the corresponding user-query conditions. Update the bind
arguments with the two additional now.toISOString() values, and merge the system
owner into dueOwners according to the existing due_at fairness ordering instead
of always prepending it.
In `@packages/worker/src/email/system-email-authority.workers.test.ts`:
- Around line 162-212: Update the legacy mirror failure test around
ensureEmailTestSchema and the ignore_system_legacy_message trigger to clean up
the trigger before the schema is rebuilt, and restore the deleted
system_email_graph_authority marker if the test removes it. Ensure subsequent
tests start with the intended email schema and authority state.
In `@packages/worker/src/email/system-email-graph-repo.node.test.ts`:
- Around line 308-343: The reconciliation test around
reconcileLegacySystemEmailGraphFromDedicated must verify the legacy mirror
outcome, not only survival of the invalid dedicated message. Add assertions that
query email_messages and confirm the cross-owner rows are mirrored, while also
asserting the reconciliation report’s referencedOwnerMismatchCount, documenting
the intended report-only behavior rather than expecting the write to be blocked.
Use the existing invalid-system-message identifiers and reconciliation result
symbols; do not change buildLegacySystemEmailGraphUpsertSql unless the intended
behavior is instead to enforce isolation.
In `@packages/worker/src/email/system-email-graph-sql.ts`:
- Around line 133-142: The legacy upsert must reject dedicated rows whose
referenced records belong to another user. In
packages/worker/src/email/system-email-graph-sql.ts:133-142, update
buildLegacySystemEmailGraphUpsertSql and each contract’s relationshipColumns
handling so source SELECTs validate referenced message, inbox, thread, and
sender identity ownership for ?1 before insertion, not only during conflict
updates; in
packages/worker/src/email/system-email-graph-repo.node.test.ts:308-360, assert
invalid-system-message is absent from email_messages and
referencedOwnerMismatchCount is non-zero.
In `@packages/worker/src/email/system-email-service.ts`:
- Around line 129-151: Update the attachment persistence flow around
insertSystemEmailAttachments to re-check attachment state when the insert
returns zero changes, using the same verification pattern as the message insert.
Only wrap the failure in RetryableInboundStorageError when the attachment rows
are still missing; allow already-completed attachments to continue to the
idempotent verification step without retrying.
In `@packages/worker/src/email/system-email.ts`:
- Around line 566-609: Update the orphan-thread cleanup in
pruneSystemEmailRetention to limit the threadRows query to
systemEmailLimits.pruneBatchSize and check deadlineMs before each deletion
chunk, stopping when the deadline is reached. Preserve chunked deletion and
result accounting so subsequent runs continue processing remaining orphan
threads.
In `@packages/worker/src/email/system-inbound-delivery-mirror.ts`:
- Around line 16-30: At module load, validate that the first two entries of
systemEmailDeliveryEventColumns are the expected identifier and message columns
before constructing insertColumns, selectColumns, and updateColumns. Fail
immediately if their order is incorrect, while preserving the existing
column-list construction for valid ordering.
In `@packages/worker/src/email/system-inbound-delivery-store.ts`:
- Around line 193-232: The operationTimestamp construction currently embeds a
random correlation token into system_email_daily_counters.updated_at,
invalidating its timestamp value. Update the surrounding
commitSystemInboundEventMutation flow to keep operationTimestamp as a valid ISO
timestamp, add or use a separate counter correlation-token column for matching
the counter update, and adjust the related INSERT, conflict update, and EXISTS
predicates accordingly.
- Around line 142-150: Update the catch block surrounding
commitSystemInboundEventMutation to log the original error before re-reading the
pointer, while preserving the existing committed-pointer return behavior and
rethrow when no pointer exists. Use the surrounding store’s established logging
mechanism so mirror-parity failures remain visible even when a pre-existing
pointer causes recovery.
In `@packages/worker/src/email/system-inbound-effect-store.ts`:
- Around line 26-45: Add NULL lease-timestamp handling in the reconciliation
query around the usage and subscription lease predicates so rows with missing
timestamps remain eligible. Also update the processing reclaim branch in the
system inbound effect store query to allow subscription effects with a NULL
lease timestamp to be retaken; apply these changes at
packages/worker/src/email/system-inbound-effect-store.ts lines 26-45 and 62-115.
---
Outside diff comments:
In `@packages/worker/src/email/inbound.ts`:
- Around line 74-92: Update the input type of rejectClaimedInboundDelivery so
authority is required rather than optional, and remove the now-unnecessary
missing-authority runtime check while preserving the existing rejection and
audit-error flow.
In `@packages/worker/src/email/service.ts`:
- Around line 473-498: Move the input.authority validation and its existing
“User inbound finalization requires Mailbox authority.” error before the try
block surrounding finalization. Keep the receive call and retryable failure
handling inside the try, so a missing authority propagates directly instead of
being wrapped as a RetryableInboundStorageError.
---
Nitpick comments:
In `@packages/worker/migrations/0131-system-email-graph-authority.sql`:
- Around line 873-897: The migration’s ON CONFLICT update can silently overwrite
an existing authority marker during re-runs. In the singleton insert, replace
the DO UPDATE behavior with DO NOTHING and add explicit re-verification that
detects and fails on any non-zero graph mismatch or provider-link count, while
preserving the existing initial count calculation.
In `@packages/worker/src/email/inbound-delivery.ts`:
- Around line 340-360: Move the `marker?.authority === 'dedicated'` rejection
decision out of the `try`/`catch`: keep only the marker query and missing-table
handling inside the `try`, then check the returned marker and throw the
compatibility error afterward. Preserve the existing return behavior when
`system_email_graph_authority` is absent.
In `@packages/worker/src/email/inbound-effects.ts`:
- Around line 743-769: Replace the error-message matching and retry in the
inbound usage recording flow with an explicit rollup capability derived once
from input.env.USAGE_EVENTS. Use that stable flag to set includeRollup when
calling recordSystemInboundUsageEffect, removing the catch-based missing-table
fallback while preserving rollup-disabled behavior when usage events are
enabled.
In `@packages/worker/src/email/reconcile-inbound-deliveries.ts`:
- Line 110: Update the reconciliation query around the projected_events CTE and
its authority_* CASE expressions: since user_id excludes system_owner_id,
replace each CASE with its ELSE column reference, then remove the now-unused
authority(system_owner_id) CTE, related join, and first bound parameter.
Preserve the remaining query semantics and parameter ordering.
In `@packages/worker/src/email/system-email-authority.ts`:
- Around line 34-60: Cache the successful authority result used by
assertSystemEmailGraphAuthority in a per-request memo keyed by the D1Database
binding, so repeated calls during one logical operation skip both queries.
Ensure the memo is reset for each request and that failed or invalid checks are
not cached, preserving the existing fail-closed behavior.
In `@packages/worker/src/email/system-email-graph-migration.node.test.ts`:
- Around line 192-221: Update graphParityDifferenceCount to import and bind
systemEmailOwnerId from ./email-owner.ts instead of embedding the 'system:email'
literal in its SQL queries, while preserving the existing legacy and dedicated
comparison behavior.
In `@packages/worker/src/email/system-email-graph-repo.ts`:
- Around line 237-245: Remove the redundant report.linkedMessageCount ===
dedicatedProviderLinkedMessageCount condition from the providerParity
calculation; the existing zero-count checks already establish this equality.
Keep the remaining providerParity conditions and classification behavior
unchanged.
- Around line 338-373: The reconcile flow performs the authority assertion
twice: once in reconcileLegacySystemEmailGraphFromDedicated and again through
loadSystemEmailGraphParityReport. If this path is optimized, add an explicit
option to loadSystemEmailGraphParityReport to skip its authority assertion and
use that option here, while preserving the default assertion behavior for other
callers.
In `@packages/worker/src/email/system-email-graph-sql.ts`:
- Around line 143-154: In the SQL generation function containing legacyColumns
and selected, assert that contract.columns[0] is exactly 'id' before
constructing the INSERT statement. Keep the existing positional column
generation unchanged when the assertion passes, and fail immediately with a
clear invariant error when a contract violates this ordering.
In `@packages/worker/src/email/system-email-graph-store.ts`:
- Around line 330-336: Update the legacy column/value construction near
legacyColumns and legacyValues to locate the id and direction entries by their
column names rather than assuming positions 0 and 1. Insert user_id at the
corresponding legacy location and derive each value from the matching source
column, preserving correct bindings if systemEmailMessageColumns is reordered.
In `@packages/worker/src/email/system-email-graph-transaction.ts`:
- Around line 96-111: Update commitSystemEmailGraphMutations and its callers,
including deleteEmptySystemEmailThreads and
pruneSystemExpiredInboundDedupePointers, to expose or consume explicit
mutation-result offsets instead of calculating positions as index * 3. Ensure
offsets account for any before statements and remain correct if
composeSystemEmailGraphMutation changes its per-mutation statement count; prefer
returning mutation results in a named structure or reusing an exported
statement-count constant.
In `@packages/worker/src/email/system-email-retention-graph.workers.test.ts`:
- Around line 74-88: Add a concise comment immediately above the recursive CTE
in the setup query explaining that the seed value plus the inclusive recursive
bound intentionally generates systemEmailLimits.maxStoredMessages + 1 rows,
matching the expectations later in the test. Leave the query and its binding
unchanged.
In `@packages/worker/src/email/system-inbound-delivery-mirror.ts`:
- Around line 50-56: Remove the redundant `column === 'message_id'` special case
in the `updateColumns` mapping and use the generic `${column} =
excluded.${column}` expression for every column, preserving the existing joined
SQL output.
In `@packages/worker/src/email/system-inbound-effect-store.ts`:
- Around line 325-340: Update the SQL query in the system inbound usage
aggregation to bind systemEmailOwnerId as a parameter instead of interpolating
it into the SELECT clause, and pass it through the existing bind call alongside
systemInboundProvider and input.months. Preserve the query’s result and
parameter ordering.
In `@packages/worker/src/email/system-inbound-transition-parity.node.test.ts`:
- Line 59: Update createDatabase to return only the sqlite instance and remove
the unused db field; ensure transition setup uses that single backing wrapper
without creating a second D1 wrapper. Preserve the existing using-based
DatabaseSync lifecycle.
🪄 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: f8b71efc-1e6f-43de-ab0a-813ab4401ad9
📒 Files selected for processing (52)
docs/contributing/architecture/authorization.mddocs/contributing/architecture/data-storage.mdpackages/worker/migrations/0131-system-email-graph-authority.sqlpackages/worker/src/admin/mailbox-maintenance.node.test.tspackages/worker/src/admin/mailbox-maintenance.tspackages/worker/src/admin/system-email-data.tspackages/worker/src/app/account-retention-dispositions.node.test.tspackages/worker/src/app/account-retention-dispositions.tspackages/worker/src/email/email-raw-mime-store.tspackages/worker/src/email/inbound-classification.tspackages/worker/src/email/inbound-delivery-transitions.tspackages/worker/src/email/inbound-delivery.tspackages/worker/src/email/inbound-effect-scheduler.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound-entitlements.workers.test.tspackages/worker/src/email/inbound-spam-controls.workers.test.tspackages/worker/src/email/inbound.tspackages/worker/src/email/legacy-system-inbound-rejection.node.test.tspackages/worker/src/email/mailbox-internal-read.node.test.tspackages/worker/src/email/mailbox-internal-read.tspackages/worker/src/email/outbound-provider-index.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/package-subscriptions.tspackages/worker/src/email/reconcile-inbound-deliveries.tspackages/worker/src/email/repo.tspackages/worker/src/email/service.tspackages/worker/src/email/system-email-authority.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/system-email-graph-migration.node.test.tspackages/worker/src/email/system-email-graph-repo.node.test.tspackages/worker/src/email/system-email-graph-repo.tspackages/worker/src/email/system-email-graph-sql.tspackages/worker/src/email/system-email-graph-store.tspackages/worker/src/email/system-email-graph-transaction.tspackages/worker/src/email/system-email-retention-graph.workers.test.tspackages/worker/src/email/system-email-service.tspackages/worker/src/email/system-email-subscriptions.workers.test.tspackages/worker/src/email/system-email.tspackages/worker/src/email/system-email.workers.test.tspackages/worker/src/email/system-inbound-delivery-authority.tspackages/worker/src/email/system-inbound-delivery-mirror.tspackages/worker/src/email/system-inbound-delivery-store.tspackages/worker/src/email/system-inbound-effect-store.tspackages/worker/src/email/system-inbound-email.tspackages/worker/src/email/system-inbound-transition-parity.node.test.tspackages/worker/src/email/test-schema.tspackages/worker/src/mcp/capabilities/admin/admin-capabilities.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.tspackages/worker/src/usage/aggregate-rollups.node.test.tspackages/worker/src/usage/aggregate-rollups.tstools/migration-ledger.json
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6d22503. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/migrations/0131-system-email-graph-authority.sql (1)
446-482: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNeither promoted-state UPDATE guards against malformed
detail_json. Both statements calljson_extractondetail_jsonwithout checking that the value parses. SQLite raisesmalformed JSONon the first bad row, which aborts the whole cutover and names no row. Nothing constrainsemail_delivery_events.detail_jsonto valid JSON.
packages/worker/migrations/0131-system-email-graph-authority.sql#L446-L482: addAND json_valid(detail_json)to the dedicated update'sWHEREclause at Line 482.packages/worker/migrations/0131-system-email-graph-authority.sql#L485-L516: add the sameAND json_valid(detail_json)condition at Line 516, so the legacy mirror projection stays identical and the parity count at Lines 518-799 does not diverge.Unparseable rows then remain unnormalized and surface as a counted mismatch, which blocks the authority marker through the existing
graph_mismatch_count = 0CHECK instead of through an opaque abort.🤖 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/migrations/0131-system-email-graph-authority.sql` around lines 446 - 482, Guard both promoted-state UPDATE statements in packages/worker/migrations/0131-system-email-graph-authority.sql at lines 446-482 and 485-516 with json_valid(detail_json) in their WHERE clauses. Ensure malformed rows remain unnormalized, are included in the existing parity mismatch count, and prevent the authority marker through the graph_mismatch_count = 0 check.
🧹 Nitpick comments (8)
packages/worker/src/test-support/system-email-graph-migration.ts (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA failing
ROLLBACKcan replace the original error.If the transaction is no longer active when the catch block runs,
db.exec('ROLLBACK')throwscannot rollback - no transaction is active. That error propagates instead of the migration failure, and the test assertion atpackages/worker/src/email/system-email-graph-migration.node.test.tsLine 858 loses the message it matches on.This does not fail today. The sentinel in migration 0131 triggers a CHECK violation, and SQLite resolves CHECK conflicts with ABORT, which reverts only the current statement and keeps the transaction open. A
RAISE(ROLLBACK, ...)trigger or anON CONFLICT ROLLBACKclause would change that.Keep the original error authoritative.
♻️ Proposed refactor
db.exec('BEGIN') try { db.exec(sql) db.exec('COMMIT') } catch (error) { - db.exec('ROLLBACK') + try { + db.exec('ROLLBACK') + } catch { + // The transaction was already resolved; keep the original error. + } throw error }🤖 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/test-support/system-email-graph-migration.ts` around lines 26 - 33, Update the transaction error handling around db.exec('ROLLBACK') so a rollback failure cannot replace the original migration error: attempt the rollback, suppress any rollback exception, then rethrow the caught error unchanged. Preserve the existing BEGIN, migration SQL execution, and COMMIT flow.packages/worker/src/email/system-email-graph-migration.node.test.ts (1)
851-860: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the rollback left no state behind.
The trigger correctly discriminates ordering. A migration that reached the thread insert would throw
graph mutation reached, which the regex at Line 859 does not match. So the preflight ordering is verified.The test does not assert the other half of the contract: after the throw, no
system_email_graph_authorityrow exists and no dedicated rows were written. That is the invariantapplyMigrationLikeD1exists to protect.💚 Proposed addition
expect(() => applyMigrationLikeD1(db, systemEmailAuthorityMigration)).toThrow( /CHECK constraint failed/u, ) + + expect( + db + .prepare( + `SELECT COUNT(*) AS count FROM sqlite_master + WHERE type = 'table' AND name = 'system_email_graph_authority'`, + ) + .get(), + ).toEqual({ count: 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/system-email-graph-migration.node.test.ts` around lines 851 - 860, Extend the test around applyMigrationLikeD1(systemEmailAuthorityMigration) to verify rollback cleanup after the expected throw: assert that system_email_graph_authority contains no rows and that no dedicated migration rows were written. Keep the existing CHECK constraint and trigger assertions unchanged.packages/worker/src/email/system-email-graph-transaction.ts (1)
240-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the result offsets instead of hardcoding
* 3.Line 253 assumes
composeSystemEmailGraphMutationemits exactly three statements per mutation. Nothing enforces that. If the composer ever emits a different count, every offset shifts silently and each caller reads another mutation's result.The failure is silent, not loud.
deleteEmptySystemEmailThreadsandpruneSystemExpiredInboundDedupePointersinpackages/worker/src/email/system-email-graph-store.tssumresult.dedicated?.meta.changesinto a returned count, andupdateSystemEmailMessageClassificationconverts it to a boolean. Wrong offsets produce wrong counts with no error.Compose once, then slice by the actual lengths.
♻️ Proposed refactor
const beforeCount = input.before?.length ?? 0 + const composed = input.mutations.map((mutation) => + composeSystemEmailGraphMutation(input.db, mutation), + ) const statements = [ ...(input.before ?? []), - ...input.mutations.flatMap((mutation) => - composeSystemEmailGraphMutation(input.db, mutation), - ), + ...composed.flat(), ...(input.after ?? []), systemEmailAuthorityGuardStatement(input.db), ] const results = await input.db.batch(statements) + let offset = beforeCount return { results, - mutationResults: input.mutations.map((_mutation, index) => { - const offset = beforeCount + index * 3 - return { - dedicated: results[offset], - legacy: results[offset + 1], - guard: results[offset + 2], - } - }), + mutationResults: composed.map((group) => { + const start = offset + offset += group.length + return { + dedicated: results[start], + legacy: results[start + 1], + guard: results[start + 2], + } + }), }🤖 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/system-email-graph-transaction.ts` around lines 240 - 260, Update the transaction assembly around composeSystemEmailGraphMutation to compose each mutation once and retain its statement array, then derive each mutation’s result slice from the accumulated actual statement lengths rather than using index * 3. Keep before and after offsets accounted for, and map dedicated, legacy, and guard results from each mutation’s corresponding slice while preserving the existing return shape.packages/worker/src/email/system-email-authority.workers.test.ts (2)
107-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the daily counter was not consumed.
chargeSystemInboundDeliveryOnceincrementssystem_email_daily_countersin abeforestatement of the same batch. The count assertion checks threads, messages, attachments, and delivery events, but not the counter. A regression that lets a rejected cross-owner delivery consume system quota would still pass this test. Add the counter to the same query.♻️ Proposed addition
(SELECT COUNT(*) FROM email_delivery_events - WHERE id = 'cross-owner-system-event') AS legacy_events`, + WHERE id = 'cross-owner-system-event') AS legacy_events, + (SELECT COUNT(*) FROM system_email_daily_counters + WHERE local_part = 'support' AND day = '2026-08-03') + AS dedicated_counters`, ).first(), ).toEqual({ ... legacy_events: 0, + dedicated_counters: 0, })Also applies to: 131-160
🤖 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/system-email-authority.workers.test.ts` around lines 107 - 129, Extend the cross-owner rejection test around chargeSystemInboundDeliveryOnce to query system_email_daily_counters alongside the existing threads, messages, attachments, and delivery-event counts. Assert the daily counter remains unchanged after the rejected delivery, covering both the primary assertion and the corresponding repeated test block.
342-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the trigger in a
finallyblock.The
DROP TRIGGERruns only when every preceding assertion in the test passes. If an assertion throws,ignore_system_legacy_messagesurvives.ensureEmailTestSchemarecreates tables but does not drop triggers, so a later test in the same isolate silently runs with a legacy-write-suppressing trigger installed. Wrap the trigger lifetime intry/finally.♻️ Proposed change
- await env.APP_DB.prepare(`DROP TRIGGER ignore_system_legacy_message`).run() + // Place the assertions above inside `try { ... }` and keep the drop here. + } finally { + await env.APP_DB.prepare( + `DROP TRIGGER IF EXISTS ignore_system_legacy_message`, + ).run() + }🤖 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/system-email-authority.workers.test.ts` at line 342, Wrap the test operations that install and exercise the ignore_system_legacy_message trigger in a try/finally block, and move the DROP TRIGGER cleanup into finally so it runs even when an assertion fails. Preserve the existing assertions and trigger behavior while ensuring the trigger is always removed.packages/worker/src/email/system-inbound-null-lease.node.test.ts (1)
16-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the migration cutoff filter explicit about lexicographic ordering.
The filter
file <= '0131-system-email-graph-authority.sql'depends on zero-padded, lexicographically sortable migration names. A future migration numbered above 999, or a renamed 0131 file, silently changes which migrations the test applies. Compare the numeric prefix instead.♻️ Proposed refactor
- for (const fileName of readdirSync(migrationsDirectory) - .filter( - (file) => - file.endsWith('.sql') && - file <= '0131-system-email-graph-authority.sql', - ) - .sort()) { + const migrationNumber = (file: string) => Number(file.slice(0, 4)) + for (const fileName of readdirSync(migrationsDirectory) + .filter((file) => file.endsWith('.sql') && migrationNumber(file) <= 131) + .sort()) {🤖 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/system-inbound-null-lease.node.test.ts` around lines 16 - 22, Update the migration filter in the test setup loop to extract and compare each filename’s numeric migration prefix against the cutoff migration number, rather than comparing full filenames lexicographically. Preserve the existing .sql-only filtering and cutoff behavior, including the 0131 boundary.packages/worker/src/email/system-email-retention-graph.workers.test.ts (1)
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the bounded batch size from the exported constant.
The test hardcodes 40 deleted threads and 20 remaining threads. Both values equal the internal orphan-thread batch limit (
systemEmailDeleteIdsMaxParameters) thatpruneSystemEmailRetentionapplies. If that limit changes, this test fails with a numeric mismatch that does not explain the cause. Import the constant and compute the expectations from the 60 seeded rows.🤖 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/system-email-retention-graph.workers.test.ts` around lines 61 - 67, Update the retention test expectations around pruneSystemEmailRetention to import and use the exported systemEmailDeleteIdsMaxParameters constant: derive deleted threads from the 60 seeded rows and the batch limit, and derive remaining orphan threads from the same values instead of hardcoding 40 and 20.packages/worker/src/email/inbound.ts (1)
113-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe runtime guard is unreachable because
authorityis now required.Line 113 types
authorityasUserInboundDeliveryAuthority, not as optional. TypeScript therefore rejects any call that omits it, and the check on line 115 can never be true for typed callers. Either drop the guard, or keep the property optional if an untyped caller can still reach this function.♻️ Proposed cleanup
authority: UserInboundDeliveryAuthority }) { - if (!input.authority) { - throw new Error('User inbound storage requires Mailbox authority.') - } const now = new Date().toISOString()🤖 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/inbound.ts` around lines 113 - 117, Resolve the unreachable guard in the function receiving the `authority: UserInboundDeliveryAuthority` input: either remove the `if (!input.authority)` validation because the property is required, or make `authority` optional if runtime callers may omit it and retain the error behavior.
🤖 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/reconcile-inbound-system-authority.workers.test.ts`:
- Line 7: Align the test named “an invalid system marker does not prevent
ordinary user reconciliation” with its intended behavior: make the ordinary user
reconciliation succeed and assert its result fields rather than the
`inbound-email-user-reconciliation-failed` warning or `usersProcessed` attempt
count. Ensure the assertions specifically demonstrate that the missing or
invalid system marker does not cause the user path to fail; if failure is
intended, rename the test and assert the expected error identity instead.
In `@packages/worker/src/email/system-inbound-rejection-store.ts`:
- Around line 53-61: Remove the separate aggregate count read and move the
detail-limit decision into the detail INSERT in
commitSystemInboundEventMutation, using a conditional predicate that rechecks
the aggregate count atomically and binds aggregateId and input.detailLimit after
the existing values. Confirm and use the mutation expectation behavior that
permits a zero-row insert when the limit has been reached.
---
Outside diff comments:
In `@packages/worker/migrations/0131-system-email-graph-authority.sql`:
- Around line 446-482: Guard both promoted-state UPDATE statements in
packages/worker/migrations/0131-system-email-graph-authority.sql at lines
446-482 and 485-516 with json_valid(detail_json) in their WHERE clauses. Ensure
malformed rows remain unnormalized, are included in the existing parity mismatch
count, and prevent the authority marker through the graph_mismatch_count = 0
check.
---
Nitpick comments:
In `@packages/worker/src/email/inbound.ts`:
- Around line 113-117: Resolve the unreachable guard in the function receiving
the `authority: UserInboundDeliveryAuthority` input: either remove the `if
(!input.authority)` validation because the property is required, or make
`authority` optional if runtime callers may omit it and retain the error
behavior.
In `@packages/worker/src/email/system-email-authority.workers.test.ts`:
- Around line 107-129: Extend the cross-owner rejection test around
chargeSystemInboundDeliveryOnce to query system_email_daily_counters alongside
the existing threads, messages, attachments, and delivery-event counts. Assert
the daily counter remains unchanged after the rejected delivery, covering both
the primary assertion and the corresponding repeated test block.
- Line 342: Wrap the test operations that install and exercise the
ignore_system_legacy_message trigger in a try/finally block, and move the DROP
TRIGGER cleanup into finally so it runs even when an assertion fails. Preserve
the existing assertions and trigger behavior while ensuring the trigger is
always removed.
In `@packages/worker/src/email/system-email-graph-migration.node.test.ts`:
- Around line 851-860: Extend the test around
applyMigrationLikeD1(systemEmailAuthorityMigration) to verify rollback cleanup
after the expected throw: assert that system_email_graph_authority contains no
rows and that no dedicated migration rows were written. Keep the existing CHECK
constraint and trigger assertions unchanged.
In `@packages/worker/src/email/system-email-graph-transaction.ts`:
- Around line 240-260: Update the transaction assembly around
composeSystemEmailGraphMutation to compose each mutation once and retain its
statement array, then derive each mutation’s result slice from the accumulated
actual statement lengths rather than using index * 3. Keep before and after
offsets accounted for, and map dedicated, legacy, and guard results from each
mutation’s corresponding slice while preserving the existing return shape.
In `@packages/worker/src/email/system-email-retention-graph.workers.test.ts`:
- Around line 61-67: Update the retention test expectations around
pruneSystemEmailRetention to import and use the exported
systemEmailDeleteIdsMaxParameters constant: derive deleted threads from the 60
seeded rows and the batch limit, and derive remaining orphan threads from the
same values instead of hardcoding 40 and 20.
In `@packages/worker/src/email/system-inbound-null-lease.node.test.ts`:
- Around line 16-22: Update the migration filter in the test setup loop to
extract and compare each filename’s numeric migration prefix against the cutoff
migration number, rather than comparing full filenames lexicographically.
Preserve the existing .sql-only filtering and cutoff behavior, including the
0131 boundary.
In `@packages/worker/src/test-support/system-email-graph-migration.ts`:
- Around line 26-33: Update the transaction error handling around
db.exec('ROLLBACK') so a rollback failure cannot replace the original migration
error: attempt the rollback, suppress any rollback exception, then rethrow the
caught error unchanged. Preserve the existing BEGIN, migration SQL execution,
and COMMIT flow.
🪄 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: d0581455-09ce-441a-a9f6-c73ef245d011
📒 Files selected for processing (28)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0131-system-email-graph-authority.sqlpackages/worker/src/email/inbound-classification.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound.tspackages/worker/src/email/reconcile-inbound-deliveries.tspackages/worker/src/email/reconcile-inbound-system-authority.workers.test.tspackages/worker/src/email/service.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/system-email-graph-migration.node.test.tspackages/worker/src/email/system-email-graph-repo.node.test.tspackages/worker/src/email/system-email-graph-repo.tspackages/worker/src/email/system-email-graph-sql.tspackages/worker/src/email/system-email-graph-store.tspackages/worker/src/email/system-email-graph-transaction.tspackages/worker/src/email/system-email-retention-graph.workers.test.tspackages/worker/src/email/system-email-service.tspackages/worker/src/email/system-email.tspackages/worker/src/email/system-email.workers.test.tspackages/worker/src/email/system-inbound-delivery-mirror.tspackages/worker/src/email/system-inbound-delivery-store.tspackages/worker/src/email/system-inbound-effect-store.tspackages/worker/src/email/system-inbound-null-lease.node.test.tspackages/worker/src/email/system-inbound-rejection-store.tspackages/worker/src/email/system-inbound-transition-parity.node.test.tspackages/worker/src/email/test-schema.tspackages/worker/src/test-support/system-email-graph-migration.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (17)
- tools/migration-ledger.json
- packages/worker/src/email/system-inbound-transition-parity.node.test.ts
- packages/worker/src/email/inbound-classification.ts
- packages/worker/src/email/system-email-graph-repo.node.test.ts
- packages/worker/src/email/system-inbound-effect-store.ts
- packages/worker/src/email/inbound-effects.ts
- packages/worker/src/email/system-email-graph-sql.ts
- packages/worker/src/email/system-email-service.ts
- packages/worker/src/email/system-inbound-delivery-mirror.ts
- packages/worker/src/email/reconcile-inbound-deliveries.ts
- packages/worker/src/email/system-email-graph-store.ts
- packages/worker/src/email/test-schema.ts
- packages/worker/src/email/system-email-graph-repo.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/src/email/system-email.workers.test.ts
- packages/worker/src/email/service.ts
- packages/worker/src/email/system-email.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>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/system-email-graph-migration-wrangler.node.test.ts`:
- Around line 62-64: Adjust the timeout configuration used by runWrangler and
the corresponding end-to-end test so the test-level timeout accommodates all
five Wrangler invocations while each spawn retains enough time to fail cleanly.
Apply the same change to both timeout occurrences referenced in the diff,
preserving the existing maxBuffer and invocation behavior.
- Around line 136-147: Update the statement-index checks around
firstGraphMutationIndex, finalNormalizationIndex, markerIndex, and
dropChecksIndex: use findLastIndex for finalNormalizationIndex, then assert all
four indexes are non-negative before evaluating ordering so missing statements
cannot pass silently.
🪄 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: 37e4ba13-090e-4c0d-beea-6d394a74e5a7
📒 Files selected for processing (9)
docs/contributing/architecture/data-storage.mdpackages/worker/migrations/0131-system-email-graph-authority.sqlpackages/worker/src/email/inbound-classification.tspackages/worker/src/email/inbound-classification.workers.test.tspackages/worker/src/email/reconcile-inbound-system-authority.workers.test.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/system-email-graph-migration-wrangler.node.test.tspackages/worker/src/email/system-inbound-rejection-store.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/migration-ledger.json
- packages/worker/src/email/inbound-classification.ts
- docs/contributing/architecture/data-storage.md
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
2 participants
Summary
Completes step 4b: dedicated
system_email_*tables become system:email read/write/retention/effect authority. Legacy shared rows remain atomic rollback mirrors until step 5.No legacy rows/tables are deleted.
System recap — extends Email and D1 authority (medium structural / high operational risk)
Mode: recap · Base:
main@802e658a· Head:860c8b30Classification: extends — moves the existing operator-email state machine to its dedicated D1 graph; no primitive added.
Primitives touched
emaild1-app-dbscheduled-cronemail-blobs-r2System map
System inbound routes to the dedicated operator graph; every transition atomically mirrors legacy rows for rollback until step 5.
Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Invariants
Verification
860c8b30: all Static, Node, Workers, MCP, E2E, preview, and aggregate checks pass; Bugbot/CodeRabbit have no substantive findingsConductor report
7787f8c9…remains recordedNote
High Risk
Large authority cutover on operator email storage and inbound lifecycle with transactional migration gates and rollback mirrors; operational mistakes or partial deploy could strand mail or break parity until repaired.
Overview
Step 4b makes the dedicated
system_email_*D1 graph the live read/write, inbound delivery, effects, retention, and admin authority forsystem:email. Legacy sharedemail_*rows stay as atomic rollback mirrors until step 5; nothing is deleted in this PR.Migration
0131-system-email-graph-authority.sqladds the cutover marker, reconciles drift since the 4a copy, normalizes inbound/dedupe state from canonicaldetail_json, and aborts the whole transaction on graph parity or zero provider-link violations before the marker is inserted. The 4b Worker requires that marker on dedicated entry points.Writes go to dedicated tables first, then mirror to legacy in the same D1 batch with parity/absence guards so a failed or silent legacy fence rolls back the batch. Admin system-email UI, internal reads, mailbox maintenance deletes, subscriptions, and reconciliation sweep dedicated tables;
system_email_graph_reconcileis rollback-only (dedicated_to_legacy). Scheduled retention prunes from the dedicated graph (R2-before-row) and drops legacy mirrors in the same batch.Inbound for operator mail is split into dedicated stores (
system-inbound-email,system-inbound-delivery-store, classification helpers); the pre-4b shared D1 system engine rejectssystem:emailonce the marker exists. System outbound and provider-index links forsystem:emailare explicitly unsupported and blocked at migration and runtime. USER Mailbox inbound paths are unchanged.Reviewed by Cursor Bugbot for commit 860c8b3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation