feat(email): drop legacy D1 email graph - #1174
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigration 0135 removes the shared USER email graph, compatibility trigger, and obsolete parity columns. Runtime writes and cleanup use dedicated system-email tables, while USER email remains Mailbox/R2-authoritative. Migration tooling, tests, administration, lifecycle logic, and recovery documentation now follow this model. ChangesLegacy email graph removal
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Conductor pre-review (acting with Kent's approval authority) — verdict: block until the approval wiring lands; everything else verified clean. Required before I merge:
Verified clean (no action needed): fail-closed CHECK gating incl. 0133 authority binding, exact frozen-count predicates, dependency/FK scans, and real-Wrangler negative tests; drop scope (system tables, provider index, sender config untouched); scaffolding removal scoped with the migration self-guarding; deletion/export/retention inventories consistent with strengthened assertions; data-storage.md reads as end state. When these land and CI is green, mark ready — merge will be immediate. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/app/account-deletion.node.test.ts (1)
833-833: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd coverage for Mailbox blob-delete failures.
The main test covers non-empty
Mailbox.listBlobReferences()data and R2 deletion beforeMailbox.purge(). Add a case that makes the blob delete fail beforepurge()so the deletion marker stays and Mailbox state remains visible for retry.🤖 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-deletion.node.test.ts` at line 833, Add a test case in the account-deletion coverage that provides non-empty Mailbox.listBlobReferences() data, forces the R2 blob deletion to fail before Mailbox.purge(), and verifies the deletion marker remains while Mailbox state stays visible for retry. Reuse the existing Mailbox and deletion setup and assertions from the main test, changing only the failure behavior and expected retained state.packages/worker/src/app/retention.ts (1)
147-147: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the removed
pruneRetentionresult fields.Source code checks show
packages/worker/src/app/retention.node.test.ts:814-816still readsresult.emailMessages, butRetentionPruneResultno longer definesemailMessagesoremailDeliveryEvents. Update this test to match the pruned result shape.🤖 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/retention.ts` at line 147, Restore emailMessages and emailDeliveryEvents on the RetentionPruneResult returned by pruneRetention, alongside usageRollups, and populate them in the result so existing retention tests can continue reading those fields.
🧹 Nitpick comments (6)
packages/worker/src/email/system-inbound-delivery-transaction.ts (1)
3-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
eventIdis required but never read.No function in this module uses
eventId. Every caller must still supply it. Either consume it, for example in an error message that identifies the failing event, or document that the field exists only for call-site clarity.🤖 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-transaction.ts` around lines 3 - 12, Update systemInboundEventMutation and its SystemInboundEventMutation input so the required eventId is either consumed in relevant error reporting or explicitly documented as call-site-only; do not leave the field unused without clarification, and preserve the existing mutation behavior.packages/worker/src/email/system-email.ts (1)
335-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the delete count by reference instead of by array index.
deletes[2]binds the message-count read to the literal position of the third statement. If a statement is added or reordered,deletedMessagessilently becomes 0. Name the statements and index by the message statement.♻️ Proposed refactor
- const deletes = await commitSystemEmailAuthorityBatch({ - db: input.db, - statements: [ - input.db - .prepare( - `DELETE FROM system_email_attachments - WHERE message_id IN (${placeholders})`, - ) - .bind(...deletableIds), - input.db - .prepare( - `DELETE FROM system_email_delivery_events - WHERE message_id IN (${placeholders})`, - ) - .bind(...deletableIds), - input.db - .prepare( - `DELETE FROM system_email_messages - WHERE id IN (${placeholders})`, - ) - .bind(...deletableIds), - ], - }) - result.deletedMessages += Number(deletes[2]?.meta.changes ?? 0) + const deleteAttachments = input.db + .prepare( + `DELETE FROM system_email_attachments + WHERE message_id IN (${placeholders})`, + ) + .bind(...deletableIds) + const deleteEvents = input.db + .prepare( + `DELETE FROM system_email_delivery_events + WHERE message_id IN (${placeholders})`, + ) + .bind(...deletableIds) + const deleteMessages = input.db + .prepare( + `DELETE FROM system_email_messages + WHERE id IN (${placeholders})`, + ) + .bind(...deletableIds) + const statements = [deleteAttachments, deleteEvents, deleteMessages] + const deletes = await commitSystemEmailAuthorityBatch({ + db: input.db, + statements, + }) + result.deletedMessages += Number( + deletes[statements.indexOf(deleteMessages)]?.meta.changes ?? 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.ts` around lines 335 - 358, Update the delete batch in the system email cleanup flow to assign the message deletion statement a named reference before calling commitSystemEmailAuthorityBatch, then read its result by that statement reference when updating result.deletedMessages instead of using deletes[2].packages/worker/src/email/inbound.ts (1)
217-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
APP_DBreassignment left over from the wrapper removal.Both sites spread an env object and then reassign
APP_DBto the same value the spread already provides. This is dead code from removing theliveUserEmailD1Database(...)wrapper call that used to occupy this position. The reassignment does not change behavior, but it is misleading — a future reader may assume some transformation happens here.
packages/worker/src/email/inbound.ts#L217-L220: drop theAPP_DB: inputEnv.APP_DBline, or replace the object literal withconst env = inputEnv.packages/worker/src/email/outbound.ts#L514-L520: drop the nestedAPP_DB: unsafeInput.env.APP_DBline insideenv: {...}.🧹 Proposed cleanup
const env = { ...inputEnv, - APP_DB: inputEnv.APP_DB, }const input: EmailSendInput = { ...unsafeInput, - env: { - ...unsafeInput.env, - APP_DB: unsafeInput.env.APP_DB, - }, + env: unsafeInput.env, }🤖 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 217 - 220, Remove the redundant APP_DB reassignment in packages/worker/src/email/inbound.ts lines 217-220, using inputEnv directly if appropriate. Also remove the nested APP_DB reassignment in packages/worker/src/email/outbound.ts lines 514-520 while preserving the surrounding env object behavior.packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts (1)
324-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason, not only a non-zero status.
runWranglersetstimeout: 60_000. IfspawnSynckills Wrangler on timeout,result.statusisnullandresult.erroris set.expect(...).not.toBe(0)then passes even though no guard rejected the migration. ThewrongCount,wrongHash,emptyObjectKey, andsystemDriftcases would silently stop testing the guards.The expired-approval case at Line 310 already asserts
'CHECK constraint failed'. Apply the same assertion to the other negative cases.♻️ Proposed helper for negative cases
+function expectGuardRejection(result: ReturnType<typeof runWrangler>) { + expect(result.error).toBeUndefined() + expect(result.status).not.toBe(0) + expect(`${result.stdout}\n${result.stderr}`).toContain( + 'CHECK constraint failed', + ) +}- const wrongCount = runWrangler(temporary.path, ...applyArguments) - expect(wrongCount.status).not.toBe(0) + expectGuardRejection(runWrangler(temporary.path, ...applyArguments))🤖 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/legacy-email-graph-drop-migration-wrangler.node.test.ts` around lines 324 - 364, Update the negative migration cases around wrongCount, wrongHash, emptyObjectKey, and systemDrift to assert Wrangler’s rejection reason, matching the existing expired-approval assertion for “CHECK constraint failed,” rather than only checking a non-zero status. Reuse the established result/error assertion pattern so timeout-induced null statuses cannot satisfy these tests.tools/apply-local-app-migrations.ts (1)
40-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetry only when the approval table is the reported cause.
The tool retries after any first failure. Migration 0134 has many other guards, for example the owner-count and parity checks. If one of those rejects the apply, this tool still writes the approval row and retries, then reports only
retryApplyon Line 73. The original failure output is discarded, so the real cause is hidden.Gate the retry on the missing-approval signal, and include
initialApplyin the final failure report.♻️ Proposed change
const initialApply = runWrangler(migrationArguments) if (initialApply.status === 0) { process.stdout.write(initialApply.stdout) process.stderr.write(initialApply.stderr) process.exit(0) } +if (!output(initialApply).includes('email_user_graph_drop_approval')) { + fail('Local APP_DB migrations failed.', initialApply) +}const retryApply = runWrangler(migrationArguments) if (retryApply.status !== 0) { fail( 'Local APP_DB migrations still failed after approval preparation.', + initialApply, + approval, retryApply, ) }🤖 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 `@tools/apply-local-app-migrations.ts` around lines 40 - 49, Update the retry flow after initialApply to proceed only when its failure output explicitly indicates the missing approval-table cause; otherwise report the original initialApply failure without inserting evidence or retrying. When the retry also fails, include both initialApply and retryApply details in the final failure report so the original cause is preserved.packages/worker/src/app/account-deletion.node.test.ts (1)
287-293: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake unexpected SQL fail the test.
After these handlers were removed, an old query can fall through to an empty result or zero-change response. A stale legacy email query can then pass this suite even though production D1 would fail on the dropped table.
Throw on unhandled test queries, or add explicit assertions that removed tables are never queried.
Proposed test-double hardening
- return { results: [] as Array<T>, meta: { changes: 0 } } + throw new Error(`Unhandled test SELECT: ${lower}`) - return { meta: { changes: 0 } } + throw new Error(`Unhandled test mutation: ${lower}`)Also applies to: 459-484
🤖 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-deletion.node.test.ts` around lines 287 - 293, Harden the SQL test double around the query-dispatch logic in account-deletion.node.test.ts so every query not matched by an explicit handler throws an error instead of returning an empty result or zero-change response. Ensure the same behavior is applied to the additional handler range noted in the comment, preserving existing results for supported queries while making stale or unexpected table access fail the test.
🤖 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 234-236: Update the export schema list near the references to the
dropped shared D1 graph and Mailbox.listBlobReferences: describe email_threads,
email_messages, email_attachments, and email_delivery_events as retired or
absent, removing the claim that their compatibility rows are deliberately
excluded.
In `@docs/contributing/mailbox-legacy-graph-drop.md`:
- Around line 38-79: Update the approval procedure around the INSERT into
email_user_graph_drop_approval to document recovery when an existing approval
has expired or a preflight failed: after creating and verifying a new immutable
backup, delete only the singleton row whose expires_at is at or before the
current time, confirm exactly one row was removed, then rerun the approval
INSERT and verification query with the new backup evidence.
In `@packages/worker/migrations/0134-drop-legacy-email-graph.sql`:
- Around line 311-339: Refine both migration_0134_legacy_email_graph_drop_guard
checks: match email_messages_delete_outbound_provider_index exactly, and
restrict view/trigger dependency detection to references involving only the
legacy email table names, excluding dedicated system_email_* tables. Replace the
table dependency sql substring checks with pragma_foreign_key_list-derived
dependencies, using normalized and anchored referenced-table identifiers.
Preserve the guard’s existing value semantics.
In
`@packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts`:
- Around line 377-410: In the test setup around the before-database snapshot,
remove the using declaration from before and retain the explicit before.close()
call. Ensure before is closed before the later after database reopening,
avoiding any automatic Symbol.dispose() close after the callback completes.
In `@packages/worker/src/email/system-email-graph-store.ts`:
- Line 388: Update insertSystemEmailAttachments around its
commitSystemEmailAuthorityBatch call to verify that every input attachment is
present in system_email_attachments after the batch, rather than relying only on
commit success or meta.changes. Preserve legitimate duplicate handling from
ignoreConflicts, but throw the existing inbound-delivery lease-loss error when
attachments are absent because the delivery fence or message reference rejected
the inserts.
In `@packages/worker/src/email/test-schema.ts`:
- Around line 239-244: Update the INSERT seed for email_user_graph_authority to
populate frozen_at and dropped_at using the same UTC ISO-8601 timestamp format
as migration 0134, including the T separator, millisecond precision, and Z
designator, instead of CURRENT_TIMESTAMP.
In `@tools/local-email-graph-drop-approval.sql`:
- Around line 19-54: The committed approval SQL must fail closed outside local
development, and the migration tool must restrict forwarded CLI options. In
tools/local-email-graph-drop-approval.sql lines 19-54, require a local-only
sentinel or otherwise make the inserted approval already expired until the local
tool explicitly refreshes it. In tools/apply-local-app-migrations.ts lines
50-60, replace unrestricted passthroughArguments forwarding with an explicit
allowlist such as --persist-to, preventing callers from supplying --remote.
---
Outside diff comments:
In `@packages/worker/src/app/account-deletion.node.test.ts`:
- Line 833: Add a test case in the account-deletion coverage that provides
non-empty Mailbox.listBlobReferences() data, forces the R2 blob deletion to fail
before Mailbox.purge(), and verifies the deletion marker remains while Mailbox
state stays visible for retry. Reuse the existing Mailbox and deletion setup and
assertions from the main test, changing only the failure behavior and expected
retained state.
In `@packages/worker/src/app/retention.ts`:
- Line 147: Restore emailMessages and emailDeliveryEvents on the
RetentionPruneResult returned by pruneRetention, alongside usageRollups, and
populate them in the result so existing retention tests can continue reading
those fields.
---
Nitpick comments:
In `@packages/worker/src/app/account-deletion.node.test.ts`:
- Around line 287-293: Harden the SQL test double around the query-dispatch
logic in account-deletion.node.test.ts so every query not matched by an explicit
handler throws an error instead of returning an empty result or zero-change
response. Ensure the same behavior is applied to the additional handler range
noted in the comment, preserving existing results for supported queries while
making stale or unexpected table access fail the test.
In `@packages/worker/src/email/inbound.ts`:
- Around line 217-220: Remove the redundant APP_DB reassignment in
packages/worker/src/email/inbound.ts lines 217-220, using inputEnv directly if
appropriate. Also remove the nested APP_DB reassignment in
packages/worker/src/email/outbound.ts lines 514-520 while preserving the
surrounding env object behavior.
In
`@packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts`:
- Around line 324-364: Update the negative migration cases around wrongCount,
wrongHash, emptyObjectKey, and systemDrift to assert Wrangler’s rejection
reason, matching the existing expired-approval assertion for “CHECK constraint
failed,” rather than only checking a non-zero status. Reuse the established
result/error assertion pattern so timeout-induced null statuses cannot satisfy
these tests.
In `@packages/worker/src/email/system-email.ts`:
- Around line 335-358: Update the delete batch in the system email cleanup flow
to assign the message deletion statement a named reference before calling
commitSystemEmailAuthorityBatch, then read its result by that statement
reference when updating result.deletedMessages instead of using deletes[2].
In `@packages/worker/src/email/system-inbound-delivery-transaction.ts`:
- Around line 3-12: Update systemInboundEventMutation and its
SystemInboundEventMutation input so the required eventId is either consumed in
relevant error reporting or explicitly documented as call-site-only; do not
leave the field unused without clarification, and preserve the existing mutation
behavior.
In `@tools/apply-local-app-migrations.ts`:
- Around line 40-49: Update the retry flow after initialApply to proceed only
when its failure output explicitly indicates the missing approval-table cause;
otherwise report the original initialApply failure without inserting evidence or
retrying. When the retry also fails, include both initialApply and retryApply
details in the final failure report so the original cause is preserved.
🪄 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: 95d84fa2-e2aa-4684-8a02-c28e638d08fb
📒 Files selected for processing (76)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mddocs/contributing/disaster-recovery.mddocs/contributing/mailbox-legacy-graph-drop.mdpackage.jsonpackages/worker/migrations/0134-drop-legacy-email-graph.sqlpackages/worker/src/account/data-targets.node.test.tspackages/worker/src/account/data-targets.tspackages/worker/src/account/export.node.test.tspackages/worker/src/account/user-owned-surfaces.node.test.tspackages/worker/src/account/user-owned-surfaces.tspackages/worker/src/admin/mailbox-maintenance.node.test.tspackages/worker/src/admin/mailbox-maintenance.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-integrations-data.node.test.tspackages/worker/src/app/account-retention-dispositions.tspackages/worker/src/app/admin-insights-data.tspackages/worker/src/app/handlers/account-email-change.node.test.tspackages/worker/src/app/handlers/auth-provider.node.test.tspackages/worker/src/app/handlers/passkeys.node.test.tspackages/worker/src/app/handlers/two-factor.node.test.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/app/retention.tspackages/worker/src/email/delivery-events.workers.test.tspackages/worker/src/email/inbound-effects.tspackages/worker/src/email/inbound-entitlements.workers.test.tspackages/worker/src/email/inbound.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tspackages/worker/src/email/legacy-system-inbound-rejection.node.test.tspackages/worker/src/email/legacy-user-email-graph-cleanup.node.test.tspackages/worker/src/email/legacy-user-email-graph-cleanup.tspackages/worker/src/email/outbound-provider-index.tspackages/worker/src/email/outbound-provider-index.workers.test.tspackages/worker/src/email/outbound.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-columns.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-health.tspackages/worker/src/email/system-email-retention-graph.workers.test.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-delivery-transaction.tspackages/worker/src/email/system-inbound-effect-store.tspackages/worker/src/email/system-inbound-rejection-store.tspackages/worker/src/email/test-schema.tspackages/worker/src/email/user-email-d1-guard.node.test.tspackages/worker/src/email/user-email-d1-guard.tspackages/worker/src/email/user-email-graph-authority.node.test.tspackages/worker/src/email/user-email-graph-authority.tspackages/worker/src/index.workers.test.tspackages/worker/src/integrations/service.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/mcp/capabilities/admin/domain.tspackages/worker/src/mcp/capabilities/integrations/integration-save.node.test.tspackages/worker/src/mcp/capabilities/openapi/openapi-binding-roundtrip.node.test.tspackages/worker/src/test-support/apply-all-migrations.tspackages/worker/src/users-test-schema.tstools/apply-local-app-migrations.tstools/local-email-graph-drop-approval.sqltools/mcp-test-support.tstools/migration-ledger.jsontools/user-email-d1-authority.node.test.tstools/user-email-d1-authority.tsvitest.mcp-e2e.config.ts
💤 Files with no reviewable changes (15)
- packages/worker/src/email/legacy-system-inbound-rejection.node.test.ts
- packages/worker/src/users-test-schema.ts
- packages/worker/src/email/system-email-graph-migration.node.test.ts
- packages/worker/src/email/system-email-graph-repo.ts
- packages/worker/src/email/system-inbound-delivery-mirror.ts
- packages/worker/src/email/system-email-graph-repo.node.test.ts
- packages/worker/src/index.workers.test.ts
- packages/worker/src/email/user-email-d1-guard.node.test.ts
- packages/worker/src/email/system-email-graph-columns.ts
- packages/worker/src/email/system-email-graph-transaction.ts
- packages/worker/src/email/system-email-graph-sql.ts
- packages/worker/src/app/account-deletion.ts
- packages/worker/src/email/user-email-d1-guard.ts
- packages/worker/src/email/legacy-user-email-graph-cleanup.ts
- packages/worker/src/email/legacy-user-email-graph-cleanup.node.test.ts
| INSERT INTO email_user_graph_drop_approval ( | ||
| singleton, | ||
| authority_frozen_at, | ||
| backup_object_key, | ||
| backup_sha256, | ||
| verified_at, | ||
| expires_at, | ||
| owner_count, | ||
| thread_count, | ||
| message_count, | ||
| attachment_count, | ||
| delivery_event_count | ||
| ) | ||
| SELECT | ||
| 1, | ||
| authority.frozen_at, | ||
| '<FRESH_BACKUP_OBJECT_KEY>', | ||
| '<FRESH_BACKUP_SHA256_LOWERCASE>', | ||
| strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), | ||
| strftime('%Y-%m-%dT%H:%M:%fZ', 'now', '+2 hours'), | ||
| 3, | ||
| 106, | ||
| 194, | ||
| 48, | ||
| 295 | ||
| FROM email_user_graph_authority authority | ||
| WHERE authority.singleton = 1 | ||
| AND authority.owner_count = 3 | ||
| AND authority.frozen_at = '2026-08-03T14:53:49Z' | ||
| AND (SELECT COUNT(*) FROM email_threads | ||
| WHERE user_id != 'system:email') = 106 | ||
| AND (SELECT COUNT(*) FROM email_messages | ||
| WHERE user_id != 'system:email') = 194 | ||
| AND ( | ||
| SELECT COUNT(*) | ||
| FROM email_attachments attachment | ||
| INNER JOIN email_messages message ON message.id = attachment.message_id | ||
| WHERE message.user_id != 'system:email' | ||
| ) = 48 | ||
| AND (SELECT COUNT(*) FROM email_delivery_events | ||
| WHERE user_id != 'system:email') = 295; | ||
| ``` |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Document an expired-approval renewal procedure.
If the approval expires before deployment, Line 38 cannot insert a fresh approval because singleton = 1 already exists. The same condition occurs after a failed preflight. The procedure then remains fail-closed without a documented recovery path.
Add a step that permits removal of only an expired approval. Require a new immutable backup and a new verification query before the operator reruns this insert.
Proposed documentation addition
-- Run only after a new backup has been downloaded and SHA-256 verified.
DELETE FROM email_user_graph_drop_approval
WHERE singleton = 1
AND julianday(expires_at) <= julianday('now');
-- Confirm that exactly one expired approval was removed, then rerun the
-- approval INSERT and verification query with the new backup evidence.
SELECT changes() = 1 AS expired_approval_removed;🤖 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/mailbox-legacy-graph-drop.md` around lines 38 - 79, Update
the approval procedure around the INSERT into email_user_graph_drop_approval to
document recovery when an existing approval has expired or a preflight failed:
after creating and verifying a new immutable backup, delete only the singleton
row whose expires_at is at or before the current time, confirm exactly one row
was removed, then rerun the approval INSERT and verification query with the new
backup evidence.
| INSERT INTO migration_0134_legacy_email_graph_drop_guard (value) | ||
| SELECT CASE WHEN COUNT(*) = 0 THEN 1 ELSE 0 END | ||
| FROM sqlite_schema | ||
| WHERE type IN ('view', 'trigger') | ||
| AND name != 'email_messages_delete_outbound_provider_index' | ||
| AND ( | ||
| lower(sql) LIKE '%email_threads%' | ||
| OR lower(sql) LIKE '%email_messages%' | ||
| OR lower(sql) LIKE '%email_attachments%' | ||
| OR lower(sql) LIKE '%email_delivery_events%' | ||
| ); | ||
|
|
||
| INSERT INTO migration_0134_legacy_email_graph_drop_guard (value) | ||
| SELECT CASE WHEN COUNT(*) = 0 THEN 1 ELSE 0 END | ||
| FROM sqlite_schema | ||
| WHERE type = 'table' | ||
| AND name NOT IN ( | ||
| 'email_threads', | ||
| 'email_messages', | ||
| 'email_attachments', | ||
| 'email_delivery_events', | ||
| 'email_inbound_usage_effects' | ||
| ) | ||
| AND ( | ||
| lower(sql) LIKE '%references email_threads%' | ||
| OR lower(sql) LIKE '%references email_messages%' | ||
| OR lower(sql) LIKE '%references email_attachments%' | ||
| OR lower(sql) LIKE '%references email_delivery_events%' | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find REFERENCES clauses to legacy tables that the LIKE guard cannot match,
# and find views/triggers naming system_email_* tables.
set -uo pipefail
fd -e sql . packages/worker/migrations | sort | while IFS= read -r file; do
# REFERENCES followed by newline, multiple spaces, or a quoted identifier
rg -nPU '(?i)references(\s{2,}|\s*\n\s*|\s*["\[`])' "$file" && echo "^^ $file"
done
echo '--- views/triggers referencing system_email_* ---'
rg -nPUi -C4 'create\s+(view|trigger)[\s\S]{0,400}?system_email_' packages/worker/migrationsRepository: kentcdodds/kody
Length of output: 636
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo '--- locate migration file and relevant guards ---'
sed -n '280,350p' packages/worker/migrations/0134-drop-legacy-email-graph.sql | cat -n
echo '--- all migration guard inserts ---'
rg -n "migration_0134_legacy_email_graph_drop_guard|DROP TABLE.*email_(threads|messages|attachments|delivery_events|inbound_usage_effects)|CREATE VIEW|CREATE TRIGGER|REFERENCES" packages/worker/migrations/0134-drop-legacy-email-graph.sql packages/worker/migrations | head -n 200
echo '--- nearby dropped legacy tables ---'
rg -n -i "email_threads|email_messages|email_attachments|email_delivery_events|email_inbound_usage_effects|system_email" packages/worker/migrations/0134-drop-legacy-email-graph.sql packages/worker/migrations | head -n 240
echo '--- schema references in all migrations for specific legacy/system tables ---'
python3 - <<'PY'
import pathlib, re
pats = [re.compile(r'(?i)\b(email_threads|email_messages|email_attachments|email_delivery_events|email_inbound_usage_effects|system_email_threads|system_email_messages|system_email_attachments|system_email_delivery_events|system_email_inbound_usage_effects)\b')]
for file in sorted(pathlib.Path('packages/worker/migrations').glob('*.sql')):
text = file.read_text(errors='replace')
for pat in pats:
if pat.search(text):
print(f"\n--- {file} ---")
for m in pat.finditer(text):
start=max(0, text.find('\n', 0, m.start())+1); end=text.find('\n', m.start(), text.find('\n', m.start())+1 if '\n' in text[m.start():] else len(text))
lo=max(0, m.start()-250); hi=min(len(text), m.end()+250)
lines = text[lo:hi].splitlines()
print(f"near {text[:m.start()].count(chr(10))+1}: {m.group()}")
for line in lines[:5]:
print(line.strip())
break
PYRepository: kentcdodds/kody
Length of output: 50372
Make the legacy email drop guards precise in both directions.
Use exact matching for email_messages_delete_outbound_provider_index, and narrow the view/trigger dependency predicates to the legacy legacy table names so triggers on dedicated system_email_* tables do not block the cutover. Derive table dependencies from pragma_foreign_key_list and use normalized/anchored checks against the referenced table identifier instead of substring sql LIKE '%references <name>%'.
🤖 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/0134-drop-legacy-email-graph.sql` around lines 311
- 339, Refine both migration_0134_legacy_email_graph_drop_guard checks: match
email_messages_delete_outbound_provider_index exactly, and restrict view/trigger
dependency detection to references involving only the legacy email table names,
excluding dedicated system_email_* tables. Replace the table dependency sql
substring checks with pragma_foreign_key_list-derived dependencies, using
normalized and anchored referenced-table identifiers. Preserve the guard’s
existing value semantics.
Source: Linters/SAST tools
| `INSERT INTO email_user_graph_authority ( | ||
| singleton, owner_count, frozen_at, max_parity_age_hours | ||
| ) VALUES (1, 0, CURRENT_TIMESTAMP, 6);`, | ||
| singleton, owner_count, frozen_at, dropped_at, backup_object_key, backup_sha256 | ||
| ) VALUES ( | ||
| 1, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, 'test/backup.sql.gz', | ||
| 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' | ||
| );`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Seed frozen_at and dropped_at in the production timestamp format.
Migration 0134 writes dropped_at with strftime('%Y-%m-%dT%H:%M:%fZ', 'now'). This seed uses CURRENT_TIMESTAMP, which produces YYYY-MM-DD HH:MM:SS with a space separator and no UTC designator. Tests that parse droppedAt or frozenAt as ISO 8601 then see a different format than production. new Date('2026-08-03 15:00:00') is parsed as local time in V8, not UTC.
Align the seed with the migration.
🐛 Proposed fix
`INSERT INTO email_user_graph_authority (
singleton, owner_count, frozen_at, dropped_at, backup_object_key, backup_sha256
) VALUES (
- 1, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, 'test/backup.sql.gz',
+ 1, 0, strftime('%Y-%m-%dT%H:%M:%fZ', 'now'),
+ strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), 'test/backup.sql.gz',
'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
);`,📝 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.
| `INSERT INTO email_user_graph_authority ( | |
| singleton, owner_count, frozen_at, max_parity_age_hours | |
| ) VALUES (1, 0, CURRENT_TIMESTAMP, 6);`, | |
| singleton, owner_count, frozen_at, dropped_at, backup_object_key, backup_sha256 | |
| ) VALUES ( | |
| 1, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, 'test/backup.sql.gz', | |
| 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' | |
| );`, | |
| `INSERT INTO email_user_graph_authority ( | |
| singleton, owner_count, frozen_at, dropped_at, backup_object_key, backup_sha256 | |
| ) VALUES ( | |
| 1, 0, strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), | |
| strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), 'test/backup.sql.gz', | |
| 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' | |
| );`, |
🤖 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/test-schema.ts` around lines 239 - 244, Update the
INSERT seed for email_user_graph_authority to populate frozen_at and dropped_at
using the same UTC ISO-8601 timestamp format as migration 0134, including the T
separator, millisecond precision, and Z designator, instead of
CURRENT_TIMESTAMP.
| INSERT INTO email_user_graph_drop_approval ( | ||
| singleton, | ||
| authority_frozen_at, | ||
| backup_object_key, | ||
| backup_sha256, | ||
| verified_at, | ||
| expires_at, | ||
| owner_count, | ||
| thread_count, | ||
| message_count, | ||
| attachment_count, | ||
| delivery_event_count | ||
| ) | ||
| SELECT | ||
| 1, | ||
| authority.frozen_at, | ||
| 'local-test/verified-d1-backup.sql.gz', | ||
| 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', | ||
| strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), | ||
| strftime('%Y-%m-%dT%H:%M:%fZ', 'now', '+1 day'), | ||
| authority.owner_count, | ||
| (SELECT COUNT(*) FROM email_threads WHERE user_id != 'system:email'), | ||
| (SELECT COUNT(*) FROM email_messages WHERE user_id != 'system:email'), | ||
| ( | ||
| SELECT COUNT(*) | ||
| FROM email_attachments attachment | ||
| INNER JOIN email_messages message ON message.id = attachment.message_id | ||
| WHERE message.user_id != 'system:email' | ||
| ), | ||
| ( | ||
| SELECT COUNT(*) | ||
| FROM email_delivery_events | ||
| WHERE user_id != 'system:email' | ||
| ) | ||
| FROM email_user_graph_authority authority | ||
| WHERE authority.singleton = 1; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fabricated drop approval can reach a non-local database. The shared root cause is that a repository-committed file writes a fully valid approval row with a fake backup key and fake SHA-256, and the local tool forwards uncontrolled CLI arguments to the command that writes it. Migration 0134 validates only the format of backup_sha256, so this row authorizes an irreversible drop of the legacy USER email graph on whichever database receives it.
tools/local-email-graph-drop-approval.sql#L19-L54: make theINSERTfail closed outside local development, for example by requiring a local-only sentinel table or by writing an already expiredexpires_atthat only the local tool refreshes.tools/apply-local-app-migrations.ts#L50-L60: stop appendingpassthroughArgumentsto thed1 executeinvocation, and forward only an explicit allowlist such as--persist-to, so a caller cannot add--remote.
📍 Affects 2 files
tools/local-email-graph-drop-approval.sql#L19-L54(this comment)tools/apply-local-app-migrations.ts#L50-L60
🤖 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 `@tools/local-email-graph-drop-approval.sql` around lines 19 - 54, The
committed approval SQL must fail closed outside local development, and the
migration tool must restrict forwarded CLI options. In
tools/local-email-graph-drop-approval.sql lines 19-54, require a local-only
sentinel or otherwise make the inserted approval already expired until the local
tool explicitly refreshes it. In tools/apply-local-app-migrations.ts lines
50-60, replace unrestricted passthroughArguments forwarding with an explicit
allowlist such as --persist-to, preventing callers from supplying --remote.
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/apply-local-app-migrations.ts (1)
47-59: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not create approval metadata after an arbitrary migration failure.
This block runs after any failed initial migration. It then inserts an approval fixture before retrying the destructive migration. If the failure came from pending repairs, parity drift, audit backlog, or dependency checks, the helper must stop without creating approval metadata.
Gate this call on the exact missing-control-plane-receipt failure, or require an explicit local test flag, before running
tools/local-mailbox-pre-drop-approval-fixture.sql.Proposed fix
+if (!isMissingControlPlaneReceiptFailure(initialMigration)) { + reportFailure('Initial migration failed', initialMigration) + process.exit(1) +} + const approval = runWrangler([🤖 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 `@tools/apply-local-app-migrations.ts` around lines 47 - 59, Restrict the approval-fixture retry in the migration helper to the specific missing control-plane receipt failure, or an explicitly enabled local test mode; do not run it after arbitrary migration errors. Update the logic around the `approval` `runWrangler` invocation to inspect the initial migration failure and stop immediately for pending repairs, parity drift, audit backlog, dependency checks, or other failures, while preserving the retry behavior for the intended fixture scenario.
🧹 Nitpick comments (1)
packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts (1)
100-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWiden the stale-reference scan beyond
system-files.The filter covers only file names that start with
system-.inbound.ts,outbound.ts,service.ts, andinbound-effects.tsalso moved to the dedicated tables in this stack, so a staleemail_messagesreference in those files still passes this test. Scan every non-test.tsfile in the directory and keep an explicit allow-list for files that legitimately name the dropped tables.🤖 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/legacy-email-graph-drop-migration-wrangler.node.test.ts` around lines 100 - 106, Update the references scan around readdirSync to inspect every non-test .ts file rather than only files beginning with system-. Add an explicit allow-list for legitimate dropped-table references, including inbound.ts, outbound.ts, service.ts, and inbound-effects.ts as appropriate, while preserving the existing exclusion for test files.
🤖 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/migrations/0135-drop-legacy-email-graph.sql`:
- Around line 592-697: Add a fail-closed column-set guard before creating or
copying into users_next, using pragma_table_info('users') ordered by cid and
comparing group_concat(name, ',') against the complete reviewed 0134 users
column order, including all 14 parity columns removed by this migration. Make
the guard insert 1 only on an exact match and 0 otherwise, alongside the
existing migration_0135_legacy_email_graph_drop_guard checks.
- Around line 16-33: Update the migration guard flow around
migration_0135_legacy_email_graph_drop_guard so databases without the legacy
email graph, including preview and newly provisioned D1 databases, bypass the
destructive preflight and drop statements without requiring a production
approval row. Add an environment-aware legacy-graph presence check that records
the absent state and makes subsequent guards and drops no-ops, while preserving
the existing approval validation and destructive behavior when the legacy graph
exists.
- Around line 480-487: Update the dependency scan query in migration 0135 to
remove the `system_` prefix from each object SQL expression before checking for
legacy table names. Apply this normalization to the `email_threads`,
`email_messages`, `email_attachments`, and `email_delivery_events` matches while
preserving the existing view/trigger filtering and excluded index condition.
In `@packages/worker/src/email/system-email-authority.workers.test.ts`:
- Around line 53-86: Update the fixture to seed foreign-owner-message with
inbox_id set to foreign-inbox, then ensure insertSystemEmailAttachments
validates the message’s inbox ownership: allow a null inbox or require the
joined email_inboxes.user_id to match systemEmailOwnerId. Preserve rejection and
verify no attachment is persisted for the mismatched owner.
In `@tools/local-mailbox-pre-drop-approval-fixture.sql`:
- Around line 10-32: In the fixture’s post-approval INSERT flow, add an
assertion immediately after the existing INSERT that attempts to insert
singleton = 2 with request_id “missing-authority-marker” and an empty nonce when
no singleton = 1 approval row exists. This must trigger the existing singleton
CHECK constraint so a missing authority marker fails loudly instead of leaving
the table empty.
---
Outside diff comments:
In `@tools/apply-local-app-migrations.ts`:
- Around line 47-59: Restrict the approval-fixture retry in the migration helper
to the specific missing control-plane receipt failure, or an explicitly enabled
local test mode; do not run it after arbitrary migration errors. Update the
logic around the `approval` `runWrangler` invocation to inspect the initial
migration failure and stop immediately for pending repairs, parity drift, audit
backlog, dependency checks, or other failures, while preserving the retry
behavior for the intended fixture scenario.
---
Nitpick comments:
In
`@packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts`:
- Around line 100-106: Update the references scan around readdirSync to inspect
every non-test .ts file rather than only files beginning with system-. Add an
explicit allow-list for legitimate dropped-table references, including
inbound.ts, outbound.ts, service.ts, and inbound-effects.ts as appropriate,
while preserving the existing exclusion for test files.
🪄 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: d237c69c-f5fd-4986-869b-18ac4d81d89d
📒 Files selected for processing (25)
docs/contributing/architecture/data-storage.mddocs/contributing/architecture/feature-flags.mddocs/contributing/disaster-recovery.mddocs/contributing/mailbox-legacy-graph-drop.mdpackages/backup-control-plane/readme.mdpackages/worker/migrations/0135-drop-legacy-email-graph.sqlpackages/worker/src/account/data-targets.tspackages/worker/src/admin/mailbox-maintenance.node.test.tspackages/worker/src/admin/mailbox-maintenance.tspackages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tspackages/worker/src/email/outbound-provider-index.tspackages/worker/src/email/outbound-provider-index.workers.test.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/system-inbound-delivery-store.tspackages/worker/src/email/system-inbound-delivery-transaction.tspackages/worker/src/email/system-inbound-rejection-store.tspackages/worker/src/email/test-schema.tspackages/worker/src/email/user-email-graph-authority.node.test.tspackages/worker/src/email/user-email-graph-authority.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.tspackages/worker/src/test-support/apply-all-migrations.tstools/apply-local-app-migrations.tstools/local-mailbox-pre-drop-approval-fixture.sqltools/migration-ledger.json
💤 Files with no reviewable changes (7)
- packages/worker/src/email/user-email-graph-authority.node.test.ts
- packages/worker/src/email/outbound-provider-index.workers.test.ts
- packages/worker/src/email/outbound-provider-index.ts
- packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.node.test.ts
- packages/worker/src/admin/mailbox-maintenance.node.test.ts
- packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.ts
- packages/worker/src/email/system-inbound-delivery-transaction.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/contributing/architecture/feature-flags.md
- packages/worker/src/email/system-inbound-delivery-store.ts
- tools/migration-ledger.json
- packages/worker/src/email/system-inbound-rejection-store.ts
- packages/worker/src/account/data-targets.ts
- docs/contributing/architecture/data-storage.md
| await insertSystemEmailAttachments({ | ||
| db: env.APP_DB, | ||
| messageId: 'foreign-owner-message', | ||
| attachments: [ | ||
| { | ||
| id: 'cross-owner-attachment', | ||
| contentType: 'text/plain', | ||
| storageKind: 'raw-mime', | ||
| }, | ||
| ], | ||
| }) | ||
| expect( | ||
| await env.APP_DB.prepare( | ||
| `SELECT id FROM system_email_attachments | ||
| WHERE id = 'cross-owner-attachment'`, | ||
| ).first(), | ||
| ).toBeNull() | ||
| await expect( | ||
| insertSystemEmailAttachments({ | ||
| db: env.APP_DB, | ||
| messageId: 'foreign-write-message', | ||
| attachments: [ | ||
| { | ||
| id: 'cross-owner-system-attachment', | ||
| storageKind: 'raw-mime', | ||
| }, | ||
| ], | ||
| }), | ||
| ).rejects.toThrow() | ||
| await expect( | ||
| chargeSystemInboundDeliveryOnce({ | ||
| recordBoundedSystemEmailRejection({ | ||
| db: env.APP_DB, | ||
| localPart: 'support', | ||
| limit: 10, | ||
| now: new Date('2026-08-03T00:00:00.000Z'), | ||
| delivery: { | ||
| deliveryId: 'cross-owner-system-event', | ||
| messageId: 'cross-owner-event-message', | ||
| threadId: 'cross-owner-event-thread', | ||
| rawMimeKey: 'email-raw:v1:system:email/cross-owner-event-message', | ||
| userId: systemEmailOwnerId, | ||
| inboxId: 'foreign-write-inbox', | ||
| recipient: 'support@example.com', | ||
| envelopeFrom: 'sender@example.net', | ||
| provider: 'cloudflare-email-routing', | ||
| quotaDay: '2026-08-03', | ||
| dedupeExpiresAt: '2026-08-05T00:00:00.000Z', | ||
| fingerprint: 'cross-owner-event-fingerprint', | ||
| state: 'pending', | ||
| }, | ||
| inboxId: 'foreign-inbox', | ||
| recipient: 'foreign@example.test', | ||
| reason: 'cross-owner', | ||
| phase: 'test', | ||
| now: new Date('2026-08-03T12:00:00.000Z'), | ||
| detailLimit: 1, | ||
| }), | ||
| ).rejects.toThrow() | ||
|
|
||
| ).rejects.toThrow('returned an invalid count') | ||
| expect( | ||
| await env.APP_DB.prepare( | ||
| `SELECT | ||
| (SELECT COUNT(*) FROM system_email_threads | ||
| WHERE id = 'cross-owner-system-thread') AS dedicated_threads, | ||
| (SELECT COUNT(*) FROM email_threads | ||
| WHERE id = 'cross-owner-system-thread') AS legacy_threads, | ||
| (SELECT COUNT(*) FROM system_email_messages | ||
| WHERE id = 'cross-owner-system-message') AS dedicated_messages, | ||
| (SELECT COUNT(*) FROM email_messages | ||
| WHERE id = 'cross-owner-system-message') AS legacy_messages, | ||
| (SELECT COUNT(*) FROM system_email_attachments | ||
| WHERE id = 'cross-owner-system-attachment') AS dedicated_attachments, | ||
| (SELECT COUNT(*) FROM email_attachments | ||
| WHERE id = 'cross-owner-system-attachment') AS legacy_attachments, | ||
| (SELECT COUNT(*) FROM system_email_delivery_events | ||
| WHERE id = 'cross-owner-system-event') AS dedicated_events, | ||
| (SELECT COUNT(*) FROM email_delivery_events | ||
| WHERE id = 'cross-owner-system-event') AS legacy_events`, | ||
| `SELECT id FROM system_email_delivery_events | ||
| WHERE inbox_id = 'foreign-inbox'`, | ||
| ).first(), | ||
| ).toEqual({ | ||
| dedicated_threads: 0, | ||
| legacy_threads: 0, | ||
| dedicated_messages: 0, | ||
| legacy_messages: 0, | ||
| dedicated_attachments: 0, | ||
| legacy_attachments: 0, | ||
| dedicated_events: 0, | ||
| legacy_events: 0, | ||
| }) | ||
| }) | ||
|
|
||
| test('dedicated system graph is the only read authority and remains user-isolated', async () => { | ||
| await ensureEmailTestSchema(env.APP_DB) | ||
| const createdAt = '2026-08-02T22:00:00.000Z' | ||
| await env.APP_DB.prepare( | ||
| `INSERT INTO email_messages ( | ||
| id, direction, user_id, from_address, subject, processing_status, | ||
| created_at, updated_at | ||
| ) VALUES ( | ||
| 'legacy-only-system', 'inbound', ?, 'legacy@example.net', | ||
| 'Must not be read', 'stored', ?, ? | ||
| )`, | ||
| ) | ||
| .bind(systemEmailOwnerId, createdAt, createdAt) | ||
| .run() | ||
| await insertSystemEmailMessage({ | ||
| db: env.APP_DB, | ||
| message: { | ||
| id: 'dedicated-system', | ||
| fromAddress: 'sender@example.net', | ||
| subject: 'Dedicated mail', | ||
| processingStatus: 'stored', | ||
| receivedAt: createdAt, | ||
| }, | ||
| }) | ||
|
|
||
| expect( | ||
| (await listSystemEmailMessages({ db: env.APP_DB, limit: 10 })).map( | ||
| (message) => message.id, | ||
| ), | ||
| ).toEqual(['dedicated-system']) | ||
| expect(await countInternalSystemEmailMessages({ env })).toBe(1) | ||
| expect( | ||
| await loadAdminSystemEmailMessageById( | ||
| env.APP_DB, | ||
| 'legacy-only-system', | ||
| env.EMAIL_BLOBS, | ||
| ), | ||
| ).toBeNull() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce attachment ownership through the message inbox.
foreign-owner-message does not exist. This test only validates the missing-parent path.
insertSystemEmailAttachments only checks that system_email_messages.id exists. It does not verify that the message inbox belongs to systemEmailOwnerId. A malformed message linked to foreign-inbox can receive a system attachment.
Seed a message with inbox_id = 'foreign-inbox' in this fixture. Assert that no attachment persists. Update insertSystemEmailAttachments to require a null inbox or an email_inboxes.user_id = systemEmailOwnerId match.
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/system-email-authority.workers.test.ts` around
lines 53 - 86, Update the fixture to seed foreign-owner-message with inbox_id
set to foreign-inbox, then ensure insertSystemEmailAttachments validates the
message’s inbox ownership: allow a null inbox or require the joined
email_inboxes.user_id to match systemEmailOwnerId. Preserve rejection and verify
no attachment is persisted for the mismatched owner.
Source: Coding guidelines
| snapshot AS ( | ||
| SELECT | ||
| authority.frozen_at AS authority_frozen_at, | ||
| authority.owner_count AS authority_owner_count, | ||
| ( | ||
| SELECT COUNT(*) FROM email_threads WHERE user_id != 'system:email' | ||
| ) AS thread_count, | ||
| ( | ||
| SELECT COUNT(*) FROM email_messages WHERE user_id != 'system:email' | ||
| ) AS message_count, | ||
| ( | ||
| SELECT COUNT(*) | ||
| FROM email_attachments attachment | ||
| INNER JOIN email_messages message ON message.id = attachment.message_id | ||
| WHERE message.user_id != 'system:email' | ||
| ) AS attachment_count, | ||
| ( | ||
| SELECT COUNT(*) FROM email_delivery_events | ||
| WHERE user_id != 'system:email' | ||
| ) AS event_count | ||
| FROM email_user_graph_authority authority | ||
| WHERE authority.singleton = 1 | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the fixture fail loudly when the authority marker is missing.
If email_user_graph_authority has no singleton = 1 row, snapshot returns no rows. The CROSS JOIN then yields no rows and the INSERT is a silent no-op. Migration 0135 later aborts with a bare CHECK constraint failed: value = 1, which gives no cause.
Add an assertion after the INSERT so local runs report the missing marker directly.
🐛 Proposed addition
-- Fail immediately when no approval row was created.
INSERT INTO email_user_graph_drop_approval (singleton, request_id, nonce)
SELECT 2, 'missing-authority-marker', ''
WHERE NOT EXISTS (
SELECT 1 FROM email_user_graph_drop_approval WHERE singleton = 1
);The singleton = 1 CHECK in 0134 then raises a clear constraint error instead of leaving the table empty.
Also applies to: 111-113
🤖 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 `@tools/local-mailbox-pre-drop-approval-fixture.sql` around lines 10 - 32, In
the fixture’s post-approval INSERT flow, add an assertion immediately after the
existing INSERT that attempts to insert singleton = 2 with request_id
“missing-authority-marker” and an empty nonce when no singleton = 1 approval row
exists. This must trigger the existing singleton CHECK constraint so a missing
authority marker fails loudly instead of leaving the table empty.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-1174.kody-a99.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/worker/src/email/system-email-graph-store.ts (1)
361-455: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAttachment insert lacks independent inbox-ownership enforcement, and its test doesn't exercise the real scenario.
insertSystemEmailAttachmentsverifies only that the referenced message exists; it relies entirely oninsertSystemEmailMessage's creation-time ownership check rather than re-verifyinginbox_idownership itself. The corresponding test does not seed a message actually linked to a foreign inbox, so it cannot detect this gap.
packages/worker/src/email/system-email-graph-store.ts#L361-L455: add an inbox-ownership check (join throughemail_inboxes.user_id = systemEmailOwnerId) to the dedicated attachment INSERT'sWHERE EXISTSclause, instead of relying solely on message existence.packages/worker/src/email/system-email-authority.workers.test.ts#L53-L65: replace the nonexistent'foreign-owner-message'reference with a message seeded directly (bypassinginsertSystemEmailMessage) withinbox_id = 'foreign-inbox', and assertinsertSystemEmailAttachmentsstill rejects it.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/system-email-graph-store.ts` around lines 361 - 455, Update insertSystemEmailAttachments in packages/worker/src/email/system-email-graph-store.ts#L361-L455 so its dedicated attachment INSERT verifies the message joins to email_inboxes with user_id = systemEmailOwnerId, in addition to message existence. Update packages/worker/src/email/system-email-authority.workers.test.ts#L53-L65 to seed a message directly with inbox_id = 'foreign-inbox' and assert insertSystemEmailAttachments rejects it, replacing the nonexistent foreign-owner-message reference.Source: Coding guidelines
🤖 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 848-851: Update the bootstrap-rule text near the authority-marker
description to enumerate the exact empty surfaces required by migration 0135,
while explicitly excluding the permitted email_user_graph_authority singleton
(singleton = 1, owner_count = 0) and healthy dedicated system authority marker.
Preserve the existing receipt and authority-marker requirements, but replace the
broad coordination-row wording with the migration’s precise marker predicates.
In `@packages/worker/migrations/0135-drop-legacy-email-graph.sql`:
- Around line 557-603: Extend the normalized_sql expression in the
schema_objects CTE with replace layers for commas, semicolons, periods, and
single quotes before the existing delimiter normalization. Preserve the current
space-padding and token-boundary probes so dependencies such as email_messages;
and email_messages.user_id are detected without matching system_email_messages.
- Around line 610-692: Replace the hardcoded per-table inserts into
migration_0135_surviving_foreign_keys with schema-derived foreign-key inventory
that enumerates every table and its referenced table from the database schema,
including future tables. Preserve the existing
migration_0135_legacy_email_graph_drop_guard users allowlist and snapshot
consumers so newly discovered child relations are included automatically.
---
Duplicate comments:
In `@packages/worker/src/email/system-email-graph-store.ts`:
- Around line 361-455: Update insertSystemEmailAttachments in
packages/worker/src/email/system-email-graph-store.ts#L361-L455 so its dedicated
attachment INSERT verifies the message joins to email_inboxes with user_id =
systemEmailOwnerId, in addition to message existence. Update
packages/worker/src/email/system-email-authority.workers.test.ts#L53-L65 to seed
a message directly with inbox_id = 'foreign-inbox' and assert
insertSystemEmailAttachments rejects it, replacing the nonexistent
foreign-owner-message reference.
🪄 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: 74de2ac7-e7fd-42cf-bed5-72410dc5edaf
📒 Files selected for processing (15)
docs/contributing/architecture/data-storage.mddocs/contributing/disaster-recovery.mddocs/contributing/mailbox-legacy-graph-drop.mdpackages/worker/migrations/0135-drop-legacy-email-graph.sqlpackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/retention.node.test.tspackages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tspackages/worker/src/email/system-email-authority.workers.test.tspackages/worker/src/email/system-email-graph-store.tspackages/worker/src/email/user-email-graph-authority.node.test.tspackages/worker/src/email/user-email-graph-authority.tspackages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.tspackages/worker/src/test-support/apply-all-migrations.tstools/apply-local-app-migrations.tstools/migration-ledger.json
💤 Files with no reviewable changes (2)
- packages/worker/src/test-support/apply-all-migrations.ts
- packages/worker/src/app/retention.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/migration-ledger.json
- packages/worker/src/mcp/capabilities/admin/admin-mailbox-maintenance.ts
| `users` columns atomically. Fresh databases may use the approval-free path only | ||
| before any shared, dedicated, provider, coordination, or idempotency row exists. | ||
| The stable authority marker retains `owner_count`, `frozen_at`, and | ||
| `dropped_at`; approved drops retain the complete canonical receipt in |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Clarify the authority-marker exception in the bootstrap rule.
Migration 0135 requires email_user_graph_authority to exist with singleton = 1 and owner_count = 0. It also requires a healthy dedicated system authority marker. Therefore, “before any ... coordination ... row exists” is too broad unless it explicitly excludes these markers. List the exact empty surfaces and marker predicates from migration 0135.
🤖 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/data-storage.md` around lines 848 - 851,
Update the bootstrap-rule text near the authority-marker description to
enumerate the exact empty surfaces required by migration 0135, while explicitly
excluding the permitted email_user_graph_authority singleton (singleton = 1,
owner_count = 0) and healthy dedicated system authority marker. Preserve the
existing receipt and authority-marker requirements, but replace the broad
coordination-row wording with the migration’s precise marker predicates.
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: 1
🧹 Nitpick comments (1)
packages/worker/migrations/0135-drop-legacy-email-graph.sql (1)
858-1416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe two hardcoded foreign-key inventories can drift apart.
Lines 858-1416 and Lines 1760-2319 repeat the same list of about 78 literal
pragma_foreign_key_listblocks. D1 forbids a non-literal argument, so the repetition is unavoidable in pure SQL. The risk is drift: if a future migration adds a table and an author updates only one of the two lists, the remaining guard silently stops covering that table.Two options reduce that risk:
- Generate both blocks from one source list in a build or test step, and assert in
packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tsthat the pre-rebuild and post-rebuild target sets are identical and match the table inventory literal at Lines 611-613.- Keep the SQL as is, and add the same set-equality assertion as a static test.
Option 2 is cheap and catches every future drift.
🤖 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/0135-drop-legacy-email-graph.sql` around lines 858 - 1416, Keep the duplicated literal pragma_foreign_key_list inventories synchronized by adding a static set-equality assertion in the migration test around the pre-rebuild and post-rebuild target sets. Parse both SQL blocks and verify they contain identical child-table targets, and that this set matches the table inventory literal used by the migration, while preserving the existing SQL lists.
🤖 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/legacy-email-graph-drop-migration-wrangler.node.test.ts`:
- Around line 398-404: Update the generated column definition in the executeSql
setup for future_stored_contract to use SQLite’s VIRTUAL mode instead of STORED,
while preserving the existing expression and migration test intent.
---
Nitpick comments:
In `@packages/worker/migrations/0135-drop-legacy-email-graph.sql`:
- Around line 858-1416: Keep the duplicated literal pragma_foreign_key_list
inventories synchronized by adding a static set-equality assertion in the
migration test around the pre-rebuild and post-rebuild target sets. Parse both
SQL blocks and verify they contain identical child-table targets, and that this
set matches the table inventory literal used by the migration, while preserving
the existing SQL lists.
🪄 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: 7b378ff2-8e80-416a-87a6-8bb00ed053b9
📒 Files selected for processing (5)
docs/contributing/disaster-recovery.mddocs/contributing/mailbox-legacy-graph-drop.mdpackages/worker/migrations/0135-drop-legacy-email-graph.sqlpackages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/migration-ledger.json
- docs/contributing/mailbox-legacy-graph-drop.md
- docs/contributing/disaster-recovery.md
| executeSql({ | ||
| workingDirectory: temporary.path, | ||
| configPath: rehearsal.configPath, | ||
| statePath: rehearsal.statePath, | ||
| sql: `ALTER TABLE users ADD COLUMN future_stored_contract TEXT | ||
| GENERATED ALWAYS AS (username || ':' || email) STORED;`, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Can SQLite ALTER TABLE ADD COLUMN add a STORED generated column, or only VIRTUAL?
💡 Result:
No, SQLite cannot add a STORED generated column using the ALTER TABLE ADD COLUMN command [1][2][3]. This operation is explicitly restricted; you can only add VIRTUAL generated columns using this command [1][4]. According to the official SQLite documentation, while both VIRTUAL and STORED generated columns are supported when creating tables, the ALTER TABLE ADD COLUMN syntax specifically prohibits the addition of STORED columns [2][4]. VIRTUAL columns remain the only type of generated column that can be added to an existing table in this manner [1][5].
Citations:
- 1: https://sqlite.org/gencol.html
- 2: https://www.sqlite.org/lang_altertable.html
- 3: https://www.sqlitetutorial.net/sqlite-generated-columns/
- 4: https://www3.sqlite.org/matrix/gencol.html
- 5: https://a1.sqlite.org/gencol.html
🌐 Web query:
SQLite ALTER TABLE ADD COLUMN STORED generated column error message
💡 Result:
In SQLite, the error message "cannot add a STORED column" occurs because the ALTER TABLE ADD COLUMN command does not support the addition of STORED generated columns [1][2][3]. While SQLite supports both VIRTUAL and STORED generated columns, ALTER TABLE is restricted to adding only VIRTUAL columns [1][4]. This limitation exists because ALTER TABLE ADD COLUMN in SQLite is designed to operate by modifying the table's schema (specifically the SQL text in the sqlite_schema table) rather than rewriting the table's content [2][5]. Adding a STORED column would require calculating and storing values for all existing rows, which falls outside the scope of the lightweight schema-only update that ALTER TABLE performs [2]. To resolve this, you have a few options: 1. Use a VIRTUAL column: If your use case allows, change the keyword from STORED to VIRTUAL (or omit the keyword entirely, as VIRTUAL is the default) [6]. 2. Recreate the table: If a STORED column is required, you must create a new table with the desired schema, copy the data from the old table to the new one, drop the old table, and rename the new table [6]. This is the standard procedure in SQLite for schema changes that are not supported by the ALTER TABLE command [7]. Top results: [1][2][6][3]
Citations:
- 1: https://sqlite.org/gencol.html
- 2: https://www.sqlite.org/lang_altertable.html
- 3: SQLite migration fails: cannot add a STORED generated column via ALTER TABLE basicmachines-co/basic-memory#561
- 4: https://a1.sqlite.org/gencol.html
- 5: https://www.sqlite.org/draft/lang_altertable.html
- 6: https://database.guide/fix-cannot-add-a-stored-column-in-sqlite/
- 7: https://sqlite.org/omitted.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'legacy-email-graph-drop-migration-wrangler.node.test.ts|executeSql|sql.*|package.json' . \
| sed 's#^\./##' | head -80
echo
echo "== exact test file exists and excerpt =="
if [ -f packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts ]; then
wc -l packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts
sed -n '350,450p;840,920p' packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts | cat -n
fi
echo
echo "== executeSql definition/usages =="
rg -n "function executeSql|const executeSql|class executeSql|export .*executeSql|executeSql\\(" packages src . \
-g '!node_modules' -g '!dist' -g '!build' | head -200Repository: kentcdodds/kody
Length of output: 6150
🏁 Script executed:
#!/bin/bash
set -u
echo "== exact test file =="
if [ -f packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts ]; then
wc -l packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts
sed -n '350,450p;840,920p' packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.ts | cat -n
else
echo "missing"
fi
echo
echo "== executeSql definition/usages =="
rg -n "executeSql" packages src . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage' | sed -n '1,200p'
echo
echo "== sqlite version references =="
rg -n "sqlite|d1|D1|cloudflare|workers" -i package.json pnpm-lock.yaml yarn.lock package-lock.json . \
-g '!node_modules' -g '!dist' -g '!build' | sed -n '1,240p'Repository: kentcdodds/kody
Length of output: 34173
Replace the STORED generated column with a VIRTUAL column.
ALTER TABLE ... ADD COLUMN cannot add a STORED generated column in SQLite; only VIRTUAL generated columns are addable. This setup can reject before applying migration 0135. If this test must cover future_stored_contract, rebuild the table when executeSql is accepted, or use the existing VIRTUAL handling here.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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/legacy-email-graph-drop-migration-wrangler.node.test.ts`
around lines 398 - 404, Update the generated column definition in the executeSql
setup for future_stored_contract to use SQLite’s VIRTUAL mode instead of STORED,
while preserving the existing expression and migration test intent.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
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 40675fe. Configure here.
| initialApply, | ||
| retryApply, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Silent zero-row approval fixture
Medium Severity
When 0135 fails the approval gate, the helper runs the local approval SQL and treats a zero exit code as success. That script can insert no rows if email_user_graph_authority is missing, yet migration still retries and fails with a generic post-approval error instead of reporting missing approval data.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 40675fe. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/local-mailbox-pre-drop-approval-fixture.sql (1)
7-12: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce the local-only boundary before emitting fixture approvals.
apply-local-app-migrations.tscan reach this fixture from anyAPP_DBtarget because it only passes--local;0135-drop-legacy-email-graph.sqlaccepts a matchingemail_user_graph_drop_approvalrow without checking a database identity. The synthetic approval satisfies the schema checks, so a non-local database with matching values can pass the destructive guard. Move the fixture behind a hard local-executor guard, reject fixture-only approvals outside local development, or bind the migration to a separately enforced database identity.🤖 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 `@tools/local-mailbox-pre-drop-approval-fixture.sql` around lines 7 - 12, The fixture approval in the fixture CTE must be usable only by a verified local executor, not merely by matching synthetic values. Add an enforced local-development guard around the fixture generation or consumption used by apply-local-app-migrations.ts and 0135-drop-legacy-email-graph.sql, rejecting fixture-only approvals for non-local APP_DB targets while preserving valid local execution.
🤖 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 `@tools/local-mailbox-pre-drop-approval-fixture.sql`:
- Around line 3-5: Update the fixture replacement flow around DELETE FROM
email_user_graph_drop_approval so the existing approval remains until a
replacement row is confirmed available. Validate that the snapshot/INSERT ...
SELECT produces a row before deleting, or perform the replacement atomically;
preserve the current row when the source query is empty, including the related
paths noted in the comment.
---
Outside diff comments:
In `@tools/local-mailbox-pre-drop-approval-fixture.sql`:
- Around line 7-12: The fixture approval in the fixture CTE must be usable only
by a verified local executor, not merely by matching synthetic values. Add an
enforced local-development guard around the fixture generation or consumption
used by apply-local-app-migrations.ts and 0135-drop-legacy-email-graph.sql,
rejecting fixture-only approvals for non-local APP_DB targets while preserving
valid local execution.
🪄 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: a685258f-0992-42a2-ad7c-cb757272360b
📒 Files selected for processing (2)
packages/worker/src/email/legacy-email-graph-drop-migration-wrangler.node.test.tstools/local-mailbox-pre-drop-approval-fixture.sql
| -- Replace the singleton so rerunning the local helper after a failed 0135 | ||
| -- attempt refreshes counts and expiry instead of colliding on its primary key. | ||
| DELETE FROM email_user_graph_drop_approval; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve the current approval until a replacement row exists.
When the authority marker is missing, snapshot is empty and the INSERT ... SELECT affects zero rows without an error. The preceding DELETE then leaves email_user_graph_drop_approval empty. Validate the snapshot before deleting, or use an atomic replacement that preserves the existing row when the source query is empty.
Also applies to: 14-36, 115-117
🤖 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 `@tools/local-mailbox-pre-drop-approval-fixture.sql` around lines 3 - 5, Update
the fixture replacement flow around DELETE FROM email_user_graph_drop_approval
so the existing approval remains until a replacement row is confirmed available.
Validate that the snapshot/INSERT ... SELECT produces a row before deleting, or
perform the replacement atomically; preserve the current row when the source
query is empty, including the related paths noted in the comment.
|
Conductor directive — execute in this exact order, immediately: You minted a valid approval at 19:10Z and let it expire at 21:10Z while iterating on review nits. Stop that loop now. Per the ship-pr skill, insignificant nits are ignorable — and nothing Bugbot has raised since CI went green is data-loss-class.
If any step fails, report the exact error in the conductor report instead of retrying silently. |
* feat(email): pass env to withAccountWriteLease at all four call sites All four email paths (inbound, inbound-effects, outbound, reconcile-inbound-deliveries) now supply env to withAccountWriteLease. This is call-site coordination only; no behavior changes beyond env/holder plumbing. Mailbox legacy graph is already dropped (#1174/#1189). Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(deletion): make withAccountWriteLease and friends UserMeter-only - withAccountWriteLease: env required; remove D1 acquire/release path, D1/DO union, waitUntil param, and active_write_count plumbing - markAccountDeleting: env required; D1 deleting_at set first (gate), then UserMeter markDeleting; no D1 lease snapshot or loading - listActiveAccountWriteLeases: (env, userId) only; UserMeter page walk; no D1 union - repairAccountWriteLease: env required; DO-only prepare/finalize; D1 audit row kept; no stale D1 clear - Remove dead waitUntil params from all call sites - D1 users.deleting_at remains the permanent point gate Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(user-meter): remove legacy authority machinery - Remove shadow types: UserMeterWriteLeaseShadow (replaced by UserMeterWriteLeaseEntry without authority field) - Remove listDoAuthorityWriteLeases, replaceLegacyWriteLeases, and assertWriteLeaseAuthority - Remove authority discriminated union behavior from acquireWriteLease, releaseWriteLease, prepareWriteLeaseRepair, finalizeWriteLeaseRepair - Keep warm authority column/shim for schema compatibility; code treats every row as authoritative DO; will drop after schema_version >= 7 - Simplify test-support/user-meter.ts to match: remove authority field from WriteLeaseRow, remove finalizeWriteLeaseRepair authority guard Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * refactor(parity): simplify deletion parity to meter-only, retire D1 lease query - DeletionParity: drop d1ActiveLeaseCount/doAuthorityLeaseCount/doLegacyLeaseCount/ tokenSetMismatches/temporaryMirrorRetired/mirrorLeaseParity; keep d1DeletingAt, meterDeletingAt, deletingAtParity, activeLeaseCount, truncated - readDeletionParity: read D1 deleting_at + UserMeter deletingAt/countActiveWriteLeases; no D1 account_write_leases query - admin-user-meter-parity: update deletionParitySchema, description, and keywords to match new DeletionParity type - Tests: remove bootstrapDeletionState/UserMeterWriteLeaseShadow, rewrite to use meter.markDeleting and new parity shape; remove split-authority tests Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * test: update tests for UserMeter-only lease authority - deletion-state.node.test.ts: rewrite for meter-only mark/list/repair; remove D1-only lease paths and waitUntil; add env-required tests; verify D1 deleting_at gate and races; add export/purge tombstone preservation test - account-deletion.node.test.ts: acquire UserMeter lease to simulate active writer; verify deletion is blocked then proceeds after lease release - user-meter.workers.test.ts: remove shadowAcquireWriteLease, listDoAuthorityWriteLeases, bootstrapDeletionState, authority field - service.node.test.ts files: remove writeLeaseDb mock hooks; batch mock runs statements directly (no D1 account_write_leases queries on runtime path) Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * docs: update write-lease docs for DO-only authority (contract complete) - account-write-lease-repair.md: remove legacy email / D1 lease paths; describe DO-only repair flow; note D1 account_write_leases quiescent - data-storage.md: note authority column warm/ignored; all callers supply env; D1 account_write_leases quiescent; write-lease rows clear on release/repair/purge - entitlements.md: rewrite Account-deletion write fencing section; mark contract complete 2026-08-03; remove split-authority/Phase-B/mirror-retired prose; update primitives table (activeLeaseCount replaces mirrorLeaseParity/doOnly) - primitives.yaml: update User meter summary to reflect authoritative DO leases Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * style: apply formatter to modified source files Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * revert: stage lease path removal after email cutover Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * style(email): format lease authority wiring Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Summary
Final destructive Mailbox contract:
system_email_*, USER Mailbox, R2, provider index, due-owner and alert signals surviveSafety gate
Migration refuses to run without direct production
email_user_graph_drop_approvalpopulated after a fresh immutable backup. Exact procedure:docs/contributing/mailbox-legacy-graph-drop.md. The recorded7787f8c9…backup is historical evidence only and cannot approve this migration.Conductor report
Note
High Risk
Destructive production migration with irreversible table drops and strict approval gates; incorrect deploy or missing receipt aborts or could leave schema/runtime mismatched if bypassed.
Overview
This PR completes the Mailbox cutover by permanently removing the shared D1
email_*graph and related rollback machinery. USER mail authority is Mailbox + R2 only; operatorsystem:emailstays on dedicatedsystem_email_*tables with no legacy dual-write.Migration
0135-drop-legacy-email-graph.sqlruns extensive atomic preflight checks (signedemail_user_graph_drop_approvalor empty bootstrap, exact USER counts, system legacy/dedicated parity, FK hygiene) before dropping graph tables, the provider-index delete trigger, Mailbox parity columns onusers, and rebuildingemail_inbound_usage_effectswithout the obsolete FK. The authority marker gainsdropped_at; the approval receipt is retained when present.Runtime cleanup: account deletion no longer calls frozen-graph privacy cleanup; export/deletion inventories drop
accountUserDataPendingDropTargetsand D1 email graph targets. Admin mailbox maintenance reportssystemEmailhealth instead of legacy parity/reconcile actions.Tooling & docs:
migrate:local/migrate:e2eusetools/apply-local-app-migrations.tsto retry 0135 with a test approval fixture when the guard fails; backup-control-plane Workflowparamsmust be a JSON object. Newdocs/contributing/mailbox-legacy-graph-drop.mdand updated architecture/DR docs describe approval, recovery, and that pre-0135 D1 is forensic evidence—not a serving source.Reviewed by Cursor Bugbot for commit 40675fe. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
New Features
Refactor