feat(inbound): Gmail OAuth connect/consent flow + initial watch arm (HT-40) - #41
Conversation
…HT-40)
The write-side that arms the Gmail push transport. Runs Google's OAuth2
authorization-code flow to obtain a mailbox's refresh token, persists it
encrypted at rest, arms the initial users.watch(), and seeds the baseline
gmail_watch_state cursor HT-41's reconcile consumer reads.
Routes (spec: specs/mail/gmail-connect.md):
- POST /api/v1/inbound/gmail/connect (Bearer-gated) -> { consentUrl }
- GET /api/v1/inbound/gmail/callback (pre-auth carve-out, signed-state CSRF
off the existing Keyring) -> verify state -> exchange code -> getProfile
-> watch() -> persist (mailbox + encrypted tokens + baseline cursor).
Nothing is persisted until the grant is proven usable.
New surface:
- specs/mail/gmail-connect.md (sibling spec; gmail-push.md xref updated)
- MailboxStore.upsertConnectedMailbox (idempotent by address; reconnect
reactivates -> active)
- GmailWatchStateStore.seedBaseline (first writer of watch_expiration)
- src/providers/adapters/gmail/watch.ts (users.watch + users.getProfile,
mirrors history.ts: injectable fetch, Bearer, AbortSignal.timeout, token
never logged)
- src/mail/gmail-connect.ts orchestration; src/api/gmail-connect.ts handlers
- gmailConnect dep is ABSENT BY DEFAULT (like gmailPush)
Internal Workspace app, no CASA; scopes gmail.readonly + gmail.send (least
privilege). Real consent + the live end-to-end proof are deferred to HT-44;
this ships against a faked Google + in-memory stores. Corrects two HT-41
comments that misattributed the baseline seed to HT-42 (it is HT-40's, per
gmail-push.md 6).
Refresh token encrypted at rest via MailboxTokenStore (verified by a test
asserting the raw ciphertext column never contains the plaintext); no token
or client_secret appears in any log line or thrown error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds the Gmail OAuth connect/consent flow, Gmail watch/profile adapter, signed callback state, encrypted token and mailbox persistence, baseline watch-state seeding, HTTP routing, and comprehensive tests. ChangesGmail connection flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant InboxAPI
participant GmailConnectService
participant Google
participant Stores
Caller->>InboxAPI: POST /api/v1/inbound/gmail/connect
InboxAPI->>GmailConnectService: beginConnect()
GmailConnectService-->>InboxAPI: consentUrl
InboxAPI-->>Caller: JSON consent URL
Caller->>Google: authorize and redirect with code/state
Google->>InboxAPI: GET /api/v1/inbound/gmail/callback
InboxAPI->>GmailConnectService: completeConnect(code, state)
GmailConnectService->>Google: exchange code and resolve profile
Google-->>GmailConnectService: tokens and mailbox profile
GmailConnectService->>Google: arm watch()
Google-->>GmailConnectService: historyId and expiration
GmailConnectService->>Stores: persist mailbox, tokens, and baseline
InboxAPI-->>Caller: HTML success page
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: 4
🤖 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/api/index.test.ts`:
- Around line 1945-1961: The expired-state test restores real timers before
invoking the callback, so the expiration check does not use the simulated time.
In the test “an expired state is rejected: 4xx html, nothing persisted,” keep
fake timers active through the api callback and its assertions, then restore
real timers afterward (or via the test cleanup hook).
In `@src/mail/gmail-connect.ts`:
- Around line 570-585: Make the Step 5 persistence flow atomic by introducing a
transaction-scoped database handle and threading it through
mailboxStore.upsertConnectedMailbox, tokenStore.upsertTokens, and
watchStateStore.seedBaseline. Execute all three operations in one transaction,
committing only when they all succeed and rolling back on any failure, while
preserving the existing values and ordering.
In `@src/store/mailboxes.test.ts`:
- Line 232: Remove the duplicate const rows declarations, retaining only one
declaration per affected block in src/store/mailboxes.test.ts (232-232) and
src/store/gmail-watch-state.test.ts (144-144, 176-176, and 195-195). Keep the
retained db.query result and existing test behavior unchanged.
- Around line 282-296: Update the test around upsertConnectedMailbox to exercise
a provider change: keep the seeded mailbox provider as one valid provider, then
reconnect using a different valid provider and assert the returned mailbox and
database row reflect the new provider. Ensure the assertion verifies the
provider is updated from EXCLUDED.provider rather than remaining unchanged.
🪄 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: 5941d180-e1f8-47e7-b816-ad8c7cd451df
📒 Files selected for processing (19)
specs/mail/gmail-connect.mdspecs/mail/gmail-push.mdsrc/api/gmail-connect.test.tssrc/api/gmail-connect.tssrc/api/gmail-webhook.test.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.tssrc/mail/gmail-connect.test.tssrc/mail/gmail-connect.tssrc/mail/gmail-reconcile.test.tssrc/mail/gmail-reconcile.tssrc/providers/adapters/gmail/index.tssrc/providers/adapters/gmail/watch.test.tssrc/providers/adapters/gmail/watch.tssrc/store/gmail-watch-state.test.tssrc/store/gmail-watch-state.tssrc/store/mailboxes.test.tssrc/store/mailboxes.ts
| expect(mailbox.status).toBe('active') | ||
| expect(typeof mailbox.id).toBe('string') | ||
|
|
||
| const rows = await db.query<{ address: string; provider: string; status: string }>( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove duplicate const rows declarations.
These repeated declarations are in the same block and cause a TypeScript redeclaration error, preventing the test files from compiling.
src/store/mailboxes.test.ts#L232-L232: retain oneconst rows = await db.query(...)declaration.src/store/gmail-watch-state.test.ts#L144-L144: retain one declaration.src/store/gmail-watch-state.test.ts#L176-L176: retain one declaration.src/store/gmail-watch-state.test.ts#L195-L195: retain one declaration.
📍 Affects 2 files
src/store/mailboxes.test.ts#L232-L232(this comment)src/store/gmail-watch-state.test.ts#L144-L144src/store/gmail-watch-state.test.ts#L176-L176src/store/gmail-watch-state.test.ts#L195-L195
🤖 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 `@src/store/mailboxes.test.ts` at line 232, Remove the duplicate const rows
declarations, retaining only one declaration per affected block in
src/store/mailboxes.test.ts (232-232) and src/store/gmail-watch-state.test.ts
(144-144, 176-176, and 195-195). Keep the retained db.query result and existing
test behavior unchanged.
Addresses CodeRabbit review on PR #41. - Persist the mailbox row, encrypted token, and baseline watch-state seed in ONE Db.transaction, so a mid-persist failure rolls back instead of leaving a partial connect (an `active` mailbox with no cursor — which would silently no-op every push the already-armed watch() delivers, worse than no mailbox). The three stores gain an optional trailing Queryable param; the token encryption boundary is untouched (encryption still happens in the method; only the statement's execution target changes). Adds a rollback test. - index.test.ts: the expired-state test kept fake timers active only until just before the request, so verifyConnectState checked wall-clock time and the 10-minute TTL boundary was never exercised. Wrap in try/finally so fake time stays active through the callback and is always restored. - mailboxes.test.ts: the reconnect provider test seeded and reconnected with the same provider, so it couldn't prove EXCLUDED.provider does anything. Seed a different provider so the update is genuinely exercised. CodeRabbit's "duplicate const rows" note was a false positive: each is in its own it() scope, and typecheck is green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the review in 1. Expired-state test used real timers before the request ( 2. Step-5 persistence not atomic ( 3. Duplicate 4. Provider-sync test didn't exercise a change ( Gates green on the new commit: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
HT-40 — Gmail OAuth connect/consent flow + initial
watch()armThe write-side that
gmail-push.mddeliberately stubbed. Everything already onmain(HT-34…HT-41) assumes a connected mailbox exists — amailboxesrow, an encrypted refresh token, an armedwatch()with a baseline cursor. Nothing created any of those. This PR does: it runs Google's OAuth2 authorization-code flow, persists the refresh token encrypted at rest, arms the initialusers.watch(), and seeds the baselinegmail_watch_statecursor HT-41's reconcile consumer reads.Design decision (signed off before coding)
Consent-UX model was confirmed with the maintainer up front — Option A:
POST /api/v1/inbound/gmail/connect— Bearer-gated, returns{ consentUrl }(the client redirects the browser; a top-level nav can't carry the Bearer header, so we don't redirect from a gated GET).GET /api/v1/inbound/gmail/callback?code&state— pre-auth carve-out (Google's redirect carries no Bearer token), exactly like the open-tracking pixel and push webhook. Authenticated by an HMAC-signedstateoff the existingKeyring(same stateless, no-session pattern as reply/view tokens; RFC 6749 §10.12 CSRF defence). Renders minimaltext/html.Spec-first
New sibling spec
specs/mail/gmail-connect.md(not agmail-push.mdsection — renumbering would break 6 existing§6/§7/§8cross-refs, and the OAuth grant lifecycle is a distinct concern).gmail-push.md§7 xref updated.Callback sequence (
gmail-connect.md§4) — nothing persisted until the grant is provenverify state → exchange code (require refresh_token) → getProfile (authoritative address) → watch() → persist (mailbox + encrypted tokens + baseline cursor). A failure before persist leaves no mailbox row / token / armed watch to clean up. Baseline cursor iswatch()'shistoryId, notgetProfile's (a test guards this with aprofile-hid-must-not-be-usedfixture).Surface
src/providers/adapters/gmail/watch.tsusers.watch+users.getProfileclient; mirrorshistory.ts(injectablefetch, Bearer,AbortSignal.timeout, token never logged)src/mail/gmail-connect.tsstatemint/verify,authorization_codeexchange,createGmailConnectServicesrc/api/gmail-connect.tsMailboxStore.upsertConnectedMailboxactive)GmailWatchStateStore.seedBaselinewatch_expiration+ baselinehistory_idgmailConnectdep is ABSENT BY DEFAULT (likegmailPush) — a deployment without its Internal OAuth app configured 404s both routes; no real composition-root wiring is added here.Sacred invariants — verified (not just claimed)
MailboxTokenStore.upsertTokens(AES-256-GCM); a test reads the rawrefresh_token_ciphertextcolumn and asserts it never contains the plaintext, whilegetTokensround-trips it back.error/error_description+ ids only; the token/code/client_secretnever enter a log line orError(grep-verified across all three new files; the callback tests assertcode/statenever appear in the rendered HTML).createWatchClient); only its interface type is imported (import type), matchinggmail-reconcile.ts.Judgment calls flagged for review
watch_failed/exchange_failed). This is a mild departure from the JSON API's "never render an upstream body" rule, chosen deliberately: it only renders to the operator who legitimately minted thestate, it's HTML-escaped + 500-char-bounded, carries no credential material, and showing why connect failed (e.g.redirect_uri_mismatch) is genuinely useful for HT-43/HT-44 setup. Happy to make it generic if preferred.getProfile()failure maps to 500, not a typed 4xx (theGmailConnectErrorenum has no slot for it and the spec's acceptance list doesn't name it). It still persists nothing (it's pre-persist).watch()failure, by contrast, is a typed 4xx.Corrects two already-merged comments
HT-41's
gmail-reconcile.tsandgmail-watch-state.tsboth attributed the baseline seed to HT-42. Pergmail-push.md§6 bullet 1 the initial arm+seed is HT-40; HT-42 only renews. Corrected here since this PR makes those comments wrong.Deferred (unchanged scope boundaries)
watch()renewal + reconciliation lease → HT-42; one-time GCP/Pub-Sub + OAuth-client provisioning → HT-43; real consent + the live end-to-end proof → HT-44 / operator. This PR is exercised entirely against a faked Google (injectedfetch) + in-memory stores — no cloud, no real grant.Gates (re-run on this commit, watched green)
npm run typecheck→ exit 0npm run lint(biome, 164 files) → exit 0npm run test→ 34 files / 675 tests passed (~90 new)🤖 Generated with Claude Code
Summary by CodeRabbit