feat(db): inbound-ingestion persistence schema — migrations 009-012 (HT-36) - #36
Merged
Merged
Conversation
…HT-36) Adds the durable state the inbound pipeline needs, all mailbox-namespaced: 009 mailboxes (isolation anchor; address UNIQUE, status CHECK); 010 mailbox_oauth_tokens (per-mailbox OAuth secrets); 011 gmail_watch_state (Gmail historyId cursor + watch expiry, kept out of the provider-agnostic mailboxes table); 012 inbound_deliveries (delivery ledger with a UNIQUE (mailbox_id, provider_message_id) claim key — the idempotency record, claim/lease, and retry queue, spec §4). SECURITY: both the refresh token AND the short-lived access token are stored as ciphertext (bytea), not plaintext — a DB dump alone must not yield usable mailbox access; HT-38 owns the crypto. inbound_deliveries.conversation_id/thread_id are ON DELETE SET NULL so the ingestion fact survives conversation deletion (invariant #1). Schema + migration tests only; no store methods or pipeline code. Implements specs/mail/inbound-ingestion.md §4 + gmail-push.md §4/§6. Gates green: typecheck, biome lint, 403 tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFour forward-only migrations add mailbox, OAuth token, Gmail watch state, and inbound delivery tables. Tests cover their constraints, foreign-key actions, migration ledger entries, idempotency, and configured-schema table expectations. The inbound-ingestion specification now records only ChangesMailbox and delivery schema
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant migrate
participant MIGRATIONS
participant Database
participant _migrations
migrate->>MIGRATIONS: Read migrations 009-012
migrate->>Database: Apply pending SQL
Database-->>migrate: Create mailbox and delivery tables
migrate->>_migrations: Record migration ids and names
migrate->>Database: Re-run migration checks
Database-->>migrate: Return no pending migrations
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/db/migrate.ts`:
- Around line 554-555: Prevent mismatched conversation and thread outcomes by
updating the delivery-outcome constraints in src/db/migrate.ts lines 554-555 so
any non-null thread_id must belong to the specified conversation_id, or remove
the redundant reference as appropriate. Add a rejection test in
src/db/migrate.test.ts lines 822-826 using valid IDs from different
conversations, verifying the insert fails.
🪄 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: e673a184-cf03-48b1-af35-cb7fc5bdc8da
📒 Files selected for processing (3)
src/db/migrate.test.tssrc/db/migrate.tssrc/db/postgres.test.ts
…t, HT-36) Two independent FKs (conversation_id, thread_id) let a ledger row pair a conversation with a thread from a DIFFERENT conversation — a corrupt outcome. A thread already belongs to exactly one conversation (threads.conversation_id NOT NULL), so thread_id alone records the outcome and the conversation is derivable; removing the redundant column makes the mismatch impossible by construction rather than merely checked. spec §4 wording and the migration test updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@specs/mail/inbound-ingestion.md`:
- Around line 110-114: Update the atomicity contract near the “claim, store
write, and outcome” statement to refer only to recording the resulting threadId,
consistent with the ledger schema and Migration 012. Remove the plural “ids”
wording and any implication that conversationId is persisted; preserve the retry
behavior of returning the stored threadId without rewriting.
🪄 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: 0784252b-e784-4e27-baaa-d4643d638a0c
📒 Files selected for processing (3)
specs/mail/inbound-ingestion.mdsrc/db/migrate.test.tssrc/db/migrate.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/db/migrate.ts
- src/db/migrate.test.ts
…act (CodeRabbit, HT-36)
Two remaining phrases in §3 step 5 and §4 ('resulting conversationId/threadId', 'resulting ids') still implied the removed conversation_id column. Both now say 'resulting threadId', consistent with migration 012.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 15, 2026
Merged
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
1 participant
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.
Implements HT-36 [C] — the inbound-ingestion persistence schema, under the HT-33 epic, against the merged spec. Schema + migration tests only — no store methods or pipeline code (later tickets).
New migrations (009–012), all mailbox-namespaced
mailboxes— the isolation anchor.addressUNIQUE (gmail-push.md §3 needs anemailAddressto resolve to exactly one mailbox);statusCHECKactive | paused | needs_reconnect;providerdeliberately un-CHECKed so a new transport adapter needs no migration.mailbox_oauth_tokens— per-mailbox OAuth secrets, 1:1 bymailbox_id. Both the refresh token and the short-lived access token are stored as ciphertext (bytea) — see the security note below. HT-38 owns the actual crypto; this only reserves the columns.gmail_watch_state— per-mailbox GmailhistoryIdcursor +watch()expiry, kept out of the provider-agnosticmailboxestable (a non-Gmail transport touches nothing here).history_idistext(Gmail's own wire type — an opaque watermark, never arithmetic'd).inbound_deliveries— the delivery ledger. UNIQUE(mailbox_id, provider_message_id)is the claim key the pipeline'sINSERT … ON CONFLICT DO NOTHINGtargets (spec §3/§4).conversation_id/thread_idareON DELETE SET NULL, not CASCADE — the ingestion fact survives even if its conversation is later removed (invariant HT-3: clean-room protocol doc #1).Tests cover every constraint, default, FK, CASCADE-vs-SET-NULL, and the exact
ON CONFLICTclaim + cross-mailbox behavior. Gates green: typecheck, biome, 403 tests.One security change I made on review (worth your eye)
The ticket/spec listed
access_tokenas a plaintext column. I changed it toaccess_token_ciphertext(bytea). An access token is itself a bearer credential granting ~1h of live mailbox access; storing it plaintext beside an encrypted refresh token would hand a DB thief that ~1h window for free, defeating the point of encrypting the refresh token. Encrypting both means a DB dump alone yields zero mailbox access without HT-38's key. Small change — flagging rather than burying it.Decisions carried through (all sound)
dead-letterspelled with a hyphen to match the spec verbatim (the ticket had a straydead_letterunderscore — the spec wins; there's a test asserting the underscore is rejected).mailbox_idas PK; the ledger gets a surrogateid+ a separate unique claim index (mirrorsthreads).status↔conversation_idnullability, and no status index beyond the claim key — both deferred to the ticket that builds the consuming store methods/queries, so we don't lock in an invariant before its edge cases are settled.🤖 Generated with Claude Code
Summary by CodeRabbit
threadIdfor outcomes and clarified retry behavior accordingly.