feat(backup): issue signed mailbox drop approvals - #1176
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughChangesMailbox pre-drop backup
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant ControlPlane
participant MailboxWorkflow
participant SourceD1
participant R2
participant ApprovalD1
AdminUI->>ControlPlane: request mailbox pre-drop backup
ControlPlane->>MailboxWorkflow: enqueue request with server-generated values
MailboxWorkflow->>SourceD1: capture and validate preflight snapshot
MailboxWorkflow->>R2: verify immutable adhoc policy
MailboxWorkflow->>SourceD1: export mailbox graph
MailboxWorkflow->>R2: verify manifest, signature, and SQL object
MailboxWorkflow->>SourceD1: compare postflight snapshot
MailboxWorkflow->>ApprovalD1: persist two-hour approval receipt
ApprovalD1-->>ControlPlane: return receipt and status
ControlPlane-->>AdminUI: render workflow status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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>
kentcdodds
marked this pull request as ready for review
August 3, 2026 18:11
Contributor
|
🔎 Preview deployed: https://kody-pr-1176.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: 6
🧹 Nitpick comments (6)
packages/shared/src/mailbox-pre-drop-approval.ts (1)
252-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected key names from
canonicalRelation.
mailboxPreDropApprovalContract.canonicalRelationdeclaresmanifestFilenameandsqlFilename, but this function hardcodes both filenames. The two definitions can drift. Read the values from the contract.♻️ Proposed refactor
- const expectedManifestKey = `${objectPrefix}/manifest.json` - const expectedSqlObjectKey = `${objectPrefix}/backup-request.sql` + const { manifestFilename, sqlFilename } = + mailboxPreDropApprovalContract.canonicalRelation + const expectedManifestKey = `${objectPrefix}/${manifestFilename}` + const expectedSqlObjectKey = `${objectPrefix}/${sqlFilename}`🤖 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/shared/src/mailbox-pre-drop-approval.ts` around lines 252 - 262, Update assertMailboxPreDropApprovalCanonicalRelation to derive the manifest and SQL object filenames from mailboxPreDropApprovalContract.canonicalRelation instead of hardcoding manifest.json and backup-request.sql, while preserving the existing objectPrefix-based key construction.packages/backup-control-plane/backup-runtime.node.test.ts (1)
103-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for the new kind gate.
This test covers the accepted legacy path.
validatePayloadalso adds a rejection path: a legacy payload withoutkindcombined withpayloadKind: 'mailbox-legacy-graph-pre-drop'must fail withinvalid-workflow-payload-kind. Add a case for that branch so the mailbox workflow cannot silently accept legacy scheduled params.🤖 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/backup-control-plane/backup-runtime.node.test.ts` around lines 103 - 138, The existing test only covers acceptance of a legacy scheduled payload without kind. Add a separate test exercising validatePayload with the same legacy payload and payloadKind set to mailbox-legacy-graph-pre-drop, and assert that runBackupRuntime rejects with invalid-workflow-payload-kind, ensuring mailbox workflows cannot accept legacy scheduled parameters.tools/ci/backup-resources.node.test.ts (1)
483-531: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a symmetric fail-closed case for insufficient lifecycle retention.
The loop tests a missing lock rule, a disabled lifecycle rule, and an under-retention lock rule (34 days). It does not test an under-retention lifecycle rule, even though
assertAdhocBackupPolicyReadbackcheckslifecycleRule.deleteObjectsTransition.condition.maxAge < adhocRetentionSecondssymmetrically to the lock check. Add a mirrored case to cover that branch.🧪 Proposed additional test case
{ lockPolicy: { rules: desired.lockPolicy.rules.map((rule) => rule.prefix === 'adhoc/' ? { ...rule, condition: { type: 'Age' as const, maxAgeSeconds: 34 * 86_400, }, } : rule, ), }, lifecyclePolicy: desired.lifecyclePolicy, }, + { + lockPolicy: desired.lockPolicy, + lifecyclePolicy: { + rules: desired.lifecyclePolicy.rules.map((rule) => + rule.conditions.prefix === 'adhoc/' + ? { + ...rule, + deleteObjectsTransition: { + condition: { + type: 'Age' as const, + maxAge: 34 * 86_400, + }, + }, + } + : rule, + ), + }, + }, ]) {🤖 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/ci/backup-resources.node.test.ts` around lines 483 - 531, Add a fourth fail-closed input to the loop in the “adhoc policy proof fails closed…” test that preserves the desired lock policy but changes the `adhoc/` lifecycle rule’s retention condition to below the required threshold, mirroring the existing under-retention lock-policy case. Ensure `assertAdhocBackupPolicyReadback` is expected to throw for this insufficient lifecycle retention case.packages/backup-control-plane/mailbox-pre-drop-runtime.ts (1)
50-107: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail fast on non-retryable BackupError inside the steps.
step.docallbacks throw plainBackupError. The Workflows engine cannot seeretryable, so a fail-closed validation error such asmailbox-pre-drop-preflight-failedormailbox-pre-drop-approval-write-failedconsumes all 3 attempts with exponential backoff beforerunconverts it. Theretries: { limit: 0 }setting on the R2 policy step already expresses the fail-fast intent for the same class of error.Wrap the step callbacks so a
BackupErrorwithretryable === falsebecomes aNonRetryableErrorat the step boundary. Retryable errors keep the current behavior.🤖 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/backup-control-plane/mailbox-pre-drop-runtime.ts` around lines 50 - 107, Update the step.do callbacks in the mailbox pre-drop flow, including mailbox-pre-drop-preflight, mailbox-pre-drop-verify-live-r2-policy, mailbox-pre-drop-postflight, mailbox-pre-drop-reread-and-verify, and mailbox-pre-drop-atomic-approval, to convert BackupError instances with retryable === false into NonRetryableError at the step boundary. Preserve retryable errors unchanged and retain the existing step retry configuration.packages/backup-control-plane/control-plane-fetch.ts (1)
164-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local
requestto avoid shadowing the handler parameter.Line 165 declares
const requestinside the case block.handleAuthenticatedalready has arequest: Requestparameter at line 74. The parameter is not used later in this block, so the current code runs correctly. Any future reference to the outerrequestinside this block would throw aReferenceErrorfrom the temporal dead zone instead of resolving to the parameter.Use a distinct name.
♻️ Proposed rename
case '/actions/mailbox-pre-drop-backup': { - const request = { + const backupRequest = { requestId: crypto.randomUUID(), nonce: randomNonce(), requestedAt: new Date().toISOString(), } - const instanceId = mailboxPreDropWorkflowInstanceId(request) + const instanceId = mailboxPreDropWorkflowInstanceId(backupRequest) await env.MAILBOX_PRE_DROP_BACKUP_WORKFLOW.create({ id: instanceId, - params: request, + params: backupRequest, })🤖 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/backup-control-plane/control-plane-fetch.ts` around lines 164 - 174, Rename the local request object in the /actions/mailbox-pre-drop-backup case within handleAuthenticated to a distinct name, and update its uses in mailboxPreDropWorkflowInstanceId and the workflow create call. Preserve the handler’s request: Request parameter without shadowing it.packages/backup-control-plane/control-plane-fetch.node.test.ts (1)
138-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the instance id embeds the generated
requestId.Line 143 checks only the
mailbox-pre-drop-prefix. The workflow rejects params whoserequestIddoes not match the instance id, per readme line 173. Bind the two values in the assertion so a future change that decouples them fails here.♻️ Proposed stricter assertion
- assert.match(created.id, /^mailbox-pre-drop-/u) + assert.equal(created.id, `mailbox-pre-drop-${String(created.params.requestId)}`) assert.doesNotMatch(JSON.stringify(created), /caller-value/u)🤖 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/backup-control-plane/control-plane-fetch.node.test.ts` around lines 138 - 144, Strengthen the instance ID assertion in the test around created.params and created.id so it verifies that created.id contains the generated requestId after the existing mailbox-pre-drop- prefix, while preserving the current parameter and caller-value assertions.
🤖 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/backup-control-plane/backup-runtime.ts`:
- Around line 153-165: Update the `mailbox-legacy-graph-pre-drop` branch to call
`mailboxPreDropRuntimePayload` with only the three approved request fields,
rather than spreading or passing the entire `payload`; preserve the existing
scheduled branch and exact-key comparison so extra mailbox keys are rejected.
- Around line 112-126: In the legacy scheduled-payload branch guarded by
!('kind' in payload), validate payload.scheduledAt before passing it to
backupPayload; reject omitted or otherwise unsupported values, including values
that would produce an unintended default Date. Preserve the existing allowedKind
check and only construct the Date and expected payload after scheduledAt
validation.
In `@packages/backup-control-plane/mailbox-pre-drop-workflow.node.test.ts`:
- Around line 382-383: Add cleanup to the test containing the consoleError
call-count assertion so the console.error spy is restored with
consoleError.mockRestore() after assertions complete. Ensure cleanup runs even
when the test fails, preserving independent exact call-count assertions in later
tests.
In `@packages/backup-control-plane/mailbox-pre-drop-workflow.ts`:
- Around line 30-35: Update the non-retryable error conversion in the catch
block to expose the original BackupError code through the workflow exit surface:
include error.code in the NonRetryableError message or assign it to a separate
machine-readable field consumed by the control-plane UI, while preserving the
existing message and retryability behavior.
In `@packages/backup-control-plane/readme.md`:
- Around line 159-162: Update the retention documentation around the adhoc/
lifecycle description to state that pre-drop manifests use the fixed
retentionTier daily while their objects remain under the adhoc/ prefix, and
clarify that the 35-day adhoc lifecycle rule applies.
In `@tools/ci/backup-resources.ts`:
- Around line 629-664: Update the error message in
assertAdhocBackupPolicyReadback to avoid directing reconciliation failures
exclusively to backup-resources-cli.ts or the raw CLOUDFLARE_API_TOKEN name.
Make the message generic about rerunning the applicable backup-resource command
with the DR backup admin token, or explicitly mention both
backup-resources-cli.ts apply and backup-resources-reconcile-cli.ts entry
points.
---
Nitpick comments:
In `@packages/backup-control-plane/backup-runtime.node.test.ts`:
- Around line 103-138: The existing test only covers acceptance of a legacy
scheduled payload without kind. Add a separate test exercising validatePayload
with the same legacy payload and payloadKind set to
mailbox-legacy-graph-pre-drop, and assert that runBackupRuntime rejects with
invalid-workflow-payload-kind, ensuring mailbox workflows cannot accept legacy
scheduled parameters.
In `@packages/backup-control-plane/control-plane-fetch.node.test.ts`:
- Around line 138-144: Strengthen the instance ID assertion in the test around
created.params and created.id so it verifies that created.id contains the
generated requestId after the existing mailbox-pre-drop- prefix, while
preserving the current parameter and caller-value assertions.
In `@packages/backup-control-plane/control-plane-fetch.ts`:
- Around line 164-174: Rename the local request object in the
/actions/mailbox-pre-drop-backup case within handleAuthenticated to a distinct
name, and update its uses in mailboxPreDropWorkflowInstanceId and the workflow
create call. Preserve the handler’s request: Request parameter without shadowing
it.
In `@packages/backup-control-plane/mailbox-pre-drop-runtime.ts`:
- Around line 50-107: Update the step.do callbacks in the mailbox pre-drop flow,
including mailbox-pre-drop-preflight, mailbox-pre-drop-verify-live-r2-policy,
mailbox-pre-drop-postflight, mailbox-pre-drop-reread-and-verify, and
mailbox-pre-drop-atomic-approval, to convert BackupError instances with
retryable === false into NonRetryableError at the step boundary. Preserve
retryable errors unchanged and retain the existing step retry configuration.
In `@packages/shared/src/mailbox-pre-drop-approval.ts`:
- Around line 252-262: Update assertMailboxPreDropApprovalCanonicalRelation to
derive the manifest and SQL object filenames from
mailboxPreDropApprovalContract.canonicalRelation instead of hardcoding
manifest.json and backup-request.sql, while preserving the existing
objectPrefix-based key construction.
In `@tools/ci/backup-resources.node.test.ts`:
- Around line 483-531: Add a fourth fail-closed input to the loop in the “adhoc
policy proof fails closed…” test that preserves the desired lock policy but
changes the `adhoc/` lifecycle rule’s retention condition to below the required
threshold, mirroring the existing under-retention lock-policy case. Ensure
`assertAdhocBackupPolicyReadback` is expected to throw for this insufficient
lifecycle retention case.
🪄 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: caf4cfb3-c232-44d9-b1dd-ffb2d0257786
📒 Files selected for processing (34)
.github/workflows/deploy.ymldocs/contributing/disaster-recovery.mddocs/contributing/environment-variables.mdpackages/backup-control-plane/backup-control-plane-test-support.tspackages/backup-control-plane/backup-policy.tspackages/backup-control-plane/backup-runtime.node.test.tspackages/backup-control-plane/backup-runtime.tspackages/backup-control-plane/backup-types.tspackages/backup-control-plane/backup-workflow.tspackages/backup-control-plane/control-plane-fetch.node.test.tspackages/backup-control-plane/control-plane-fetch.tspackages/backup-control-plane/control-plane-ui.tspackages/backup-control-plane/mailbox-pre-drop-approval.node.test.tspackages/backup-control-plane/mailbox-pre-drop-approval.tspackages/backup-control-plane/mailbox-pre-drop-persistence.tspackages/backup-control-plane/mailbox-pre-drop-policy.tspackages/backup-control-plane/mailbox-pre-drop-r2-policy.tspackages/backup-control-plane/mailbox-pre-drop-runtime.tspackages/backup-control-plane/mailbox-pre-drop-snapshot.tspackages/backup-control-plane/mailbox-pre-drop-verification.tspackages/backup-control-plane/mailbox-pre-drop-workflow.node.test.tspackages/backup-control-plane/mailbox-pre-drop-workflow.tspackages/backup-control-plane/readme.mdpackages/backup-control-plane/worker.tspackages/backup-control-plane/workflow-trigger.node.test.tspackages/backup-control-plane/workflow-trigger.tspackages/backup-control-plane/wrangler.jsoncpackages/shared/src/mailbox-pre-drop-approval.tspackages/worker/migrations/0134-email-user-graph-drop-approval.sqlpackages/worker/src/email/user-graph-drop-approval-migration.node.test.tstools/ci/backup-resources-reconcile-cli.tstools/ci/backup-resources.node.test.tstools/ci/backup-resources.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}
Contributor
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 07128ae. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Non-destructive prerequisite for the final legacy email graph drop:
adhoc/The destructive drop becomes migration 0135 and trusts only this control-plane-issued row.
System recap — extends backup control plane (medium risk)
Mode: recap · Base:
main@96e0dc58· Head:039c2965Classification: extends — adds a signed pre-drop backup/approval workflow to the existing production backup primitive.
Primitives touched
backup-control-planed1-app-dbemailSystem map
The DR control plane exports and verifies the frozen production D1 snapshot, then atomically issues the only approval migration 0135 will accept.
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
adhoc/35-day lock and lifecycle were manually applied and read back on 2026-08-03; deployment optionally reconciles drift, runtime probe is the canonical trigger gateConductor report
Note
Medium Risk
Extends disaster-recovery and production D1 approval paths with new Workflows, atomic D1 writes, and optional deploy-time R2 policy reconciliation; mistakes could block a legitimate drop or weaken immutability checks, but the change is explicitly non-destructive and heavily gated.
Overview
Adds a non-destructive prerequisite for dropping the frozen USER legacy email graph: the DR backup control plane can run a dedicated Workflow that exports D1, verifies integrity, and atomically writes a short-lived singleton approval row—nothing in this PR drops data.
New Workflow and storage lane.
kody-mailbox-legacy-graph-pre-drop-backupaccepts only server-generatedrequestId,nonce, andrequestedAt. It stores request-unique immutable SQL and a signed v2 manifest underadhoc/mailbox-drop/d1/...(35-day lock/lifecycle), not under daily prefixes. Migration 0134 creates onlyemail_user_graph_drop_approvalwith CHECK constraints tying keys, hashes, and provenance to that contract.Fail-closed gates. Before export, the Workflow probes live R2 (destructive overwrite must be rejected). It snapshots frozen-authority markers and exact owner/thread/message/attachment/event counts pre- and post-export, re-reads manifest and SQL from R2, verifies Ed25519 signatures, then UPSERTs approval only if the snapshot still matches—monotonic replay for the same request, no caller-supplied evidence.
Control plane UI and deploy. Access-protected
POST /actions/mailbox-pre-drop-backupand status polling enqueue the Workflow with generated params only. Scheduled backup payloads gain akinddiscriminator with legacy compatibility for persisted payloads withoutkind. Production deploy optionally runsbackup-resources-reconcile-cliwhenDR_BACKUP_ADMIN_TOKENis set; without it, deploy logs a skip while the runtime R2 probe remains the canonical gate.adhoc/retention is added to backup resource provisioning and path filters for control-plane deploys.Reviewed by Cursor Bugbot for commit e8ae483. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation