feat(oauth): Gmail OAuth token persistence + refresh service (HT-38) - #37
Conversation
AES-256-GCM token-crypto (src/store) + MailboxTokenStore (encrypt-at-rest over mailbox_oauth_tokens) + createGmailOAuthTokenService (getAccessToken: cache or refresh against Google's token endpoint; invalid_grant -> mailbox needs_reconnect; blank-refresh_token clobber guard) + minimal MailboxStore.markNeedsReconnect. Both refresh AND access tokens stored encrypted (HT-36 schema). token-crypto placed under src/store (its only consumer) so the store doesn't import mail/. New env: HELPTHREAD_TOKEN_ENC_KEY, GMAIL_OAUTH_CLIENT_ID/SECRET (composition-root injected, never hardcoded/logged). typecheck + biome clean; the ticket's own 50 tests pass in isolation (full suite via CI — a local full run hit sibling-worktree contention). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds encrypted mailbox OAuth token storage, mailbox reconnect status updates, and a Gmail OAuth service that returns cached access tokens or refreshes and persists new ones through Google’s token endpoint. ChangesGmail OAuth token lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GmailOAuthTokenService
participant MailboxTokenStore
participant GoogleTokenEndpoint
participant MailboxStore
Caller->>GmailOAuthTokenService: getAccessToken(mailboxId)
GmailOAuthTokenService->>MailboxTokenStore: getTokens(mailboxId)
alt Cached token is fresh
MailboxTokenStore-->>GmailOAuthTokenService: stored access token
GmailOAuthTokenService-->>Caller: access token
else Token requires refresh
GmailOAuthTokenService->>GoogleTokenEndpoint: POST refresh-token form
GoogleTokenEndpoint-->>GmailOAuthTokenService: refreshed token or error
alt Refresh succeeds
GmailOAuthTokenService->>MailboxTokenStore: upsertTokens(updated tokens)
GmailOAuthTokenService-->>Caller: new access token
else invalid_grant
GmailOAuthTokenService->>MailboxStore: markNeedsReconnect(mailboxId)
GmailOAuthTokenService-->>Caller: reconnect-required error
end
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mail/gmail-oauth.test.ts (1)
159-270: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for blank (empty-string)
refresh_token/scopein the response.Existing tests only cover the "field omitted" case for rotation/carry-forward (lines 159-202, 227-248, 250-270). The guard at
gmail-oauth.tslines 301-308 explicitly also handles the "field present but empty string" case — untested here, despite being a stated PR objective (preventing blank tokens from clobbering stored ones).✅ Suggested additional test
+ it('keeps the existing refresh token when the response returns an empty-string refresh_token', async () => { + const { db, tokenStore, mailboxStore } = await freshStores() + const mailboxId = await insertMailbox(db) + await tokenStore.upsertTokens(mailboxId, { refreshToken: 'old-refresh-token' }) + const { fetchImpl } = fakeTokenEndpoint(200, { + access_token: 'fresh-access-token', + expires_in: 3600, + refresh_token: '', + }) + const service = createGmailOAuthTokenService({ + tokenStore, + mailboxStore, + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + fetchImpl, + }) + + await service.getAccessToken(mailboxId) + + const stored = await tokenStore.getTokens(mailboxId) + expect(stored?.refreshToken).toBe('old-refresh-token') + })🤖 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/mail/gmail-oauth.test.ts` around lines 159 - 270, Add test coverage in the Gmail OAuth tests for refresh responses containing empty-string refresh_token and scope values. Verify getAccessToken preserves the previously stored refresh token and scopes, matching the existing omitted-field rotation and carry-forward tests and covering the guards in the token refresh handling.
🤖 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.
Nitpick comments:
In `@src/mail/gmail-oauth.test.ts`:
- Around line 159-270: Add test coverage in the Gmail OAuth tests for refresh
responses containing empty-string refresh_token and scope values. Verify
getAccessToken preserves the previously stored refresh token and scopes,
matching the existing omitted-field rotation and carry-forward tests and
covering the guards in the token refresh handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a54071b-e70e-4674-bd52-40f6d07b7ab1
📒 Files selected for processing (9)
src/mail/gmail-oauth.test.tssrc/mail/gmail-oauth.tssrc/store/index.tssrc/store/mailbox-tokens.test.tssrc/store/mailbox-tokens.tssrc/store/mailboxes.test.tssrc/store/mailboxes.tssrc/store/token-crypto.test.tssrc/store/token-crypto.ts
Implements HT-38 [E] — the Gmail OAuth token persistence + refresh service, under the HT-33 epic. Turns a stored, encrypted refresh token into the live access token the Gmail adapters need.
What's here
src/store/token-crypto.ts— AES-256-GCM authenticated encryption. Fresh CSPRNG IV per call, full 128-bit auth tag,decryptthrows on any tamper (wrong key / corrupted bytes), key injected from env and never hardcoded or logged. Wire formativ‖tag‖ciphertextin onebytea.src/store/mailbox-tokens.ts—MailboxTokenStore: encrypt-at-rest CRUD overmailbox_oauth_tokens(migration 010). Plaintext in/out, ciphertext in the DB.src/mail/gmail-oauth.ts—createGmailOAuthTokenService:getAccessToken(mailboxId)returns a cached token when fresh, else refreshes against Google's token endpoint (injectedfetch, so tests never hit Google).invalid_grant→ mailboxneeds_reconnect(operator-actionable, not a crash); other failures throw without touching status. Guards a blankrefresh_tokenfrom clobbering a good stored one.src/store/mailboxes.ts— minimalMailboxStore.markNeedsReconnect(HT-42 reuses it).Both OAuth secrets (refresh and access token) are stored encrypted — HT-36's schema decision realized here.
Review notes
token-crypto.tsinsrc/mail/, but its only consumer is the store — I moved it tosrc/store/so the store doesn't import frommail/.HELPTHREAD_TOKEN_ENC_KEY(base64 32-byte AES key),GMAIL_OAUTH_CLIENT_ID,GMAIL_OAUTH_CLIENT_SECRET.typecheck+biomeclean; the ticket's own 50 tests pass in isolation. A full-suite run locally hit flaky failures from three sibling worktrees sharingnode_modulesduring concurrent agent test runs — CI runs the suite isolated, which is the authoritative check.Scope: token persistence + refresh + crypto only — no connect/consent flow (HT-40), webhook (HT-39), or
watch()(HT-42).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests