feat(mail): Gmail OAuth disconnect admin action (HT-47) - #46
Merged
Conversation
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an authenticated Gmail disconnect endpoint and service that stops watches, revokes OAuth tokens, and performs transactional local cleanup. Introduces the ChangesGmail Disconnect
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant InboxAPI
participant GmailDisconnectService
participant GmailWatchClient
participant Google
participant Database
Operator->>InboxAPI: POST disconnect with address
InboxAPI->>GmailDisconnectService: disconnect(address)
GmailDisconnectService->>GmailWatchClient: stop watch
GmailWatchClient->>Google: POST users.stop
GmailDisconnectService->>Google: revoke refresh token
GmailDisconnectService->>Database: mark disconnected and delete credentials/watch state
GmailDisconnectService-->>InboxAPI: disconnect result
InboxAPI-->>Operator: JSON response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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/mail/gmail-disconnect.ts`:
- Around line 204-254: Serialize the lifecycle transition in disconnect around
the visible disconnect flow, including mailbox/token reads, network cleanup, and
the final transaction, using the same mailbox-level lock or atomic lifecycle
claim used by connect. Ensure concurrent reconnects cannot have fresh tokens or
watch state deleted, and ensure disconnect cannot be overwritten by a reconnect;
add a test covering both commit interleavings.
- Around line 103-114: Remove response-body extraction and concatenation from
the non-OK branch of revokeToken, so the thrown error includes only the status
and statusText. Add a fixture with a token-containing error body and verify that
neither the resulting exception nor logs expose the token, preserving the
module’s token-secrecy guarantee.
In `@src/store/mailboxes.test.ts`:
- Around line 247-259: The markDisconnected transaction test must verify
rollback participation rather than only the committed status. Update the test
around store.markDisconnected to throw after invoking it within the supplied
transaction, assert the transaction rejects, then query the mailbox and confirm
its status remains needs_reconnect.
🪄 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: 1e7768c3-bc82-40ee-a6b1-43899680cf61
📒 Files selected for processing (25)
specs/mail/gmail-connect.mdsrc/api/gmail-disconnect.test.tssrc/api/gmail-disconnect.tssrc/api/gmail-webhook.test.tssrc/api/index.test.tssrc/api/index.tssrc/api/router.test.tssrc/api/router.tssrc/composition/root.test.tssrc/composition/root.tssrc/db/migrate.test.tssrc/db/migrate.tssrc/mail/gmail-connect.test.tssrc/mail/gmail-disconnect.test.tssrc/mail/gmail-disconnect.tssrc/mail/gmail-reconcile.test.tssrc/mail/gmail-watch-maintenance.test.tssrc/providers/adapters/gmail/watch.test.tssrc/providers/adapters/gmail/watch.tssrc/store/gmail-watch-state.test.tssrc/store/gmail-watch-state.tssrc/store/mailbox-tokens.test.tssrc/store/mailbox-tokens.tssrc/store/mailboxes.test.tssrc/store/mailboxes.ts
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/mail/gmail-oauth.ts`:
- Around line 345-353: Make the token persistence in the mailbox token-update
flow atomic with the mailbox status check: replace the separate
getMailboxById/upsertTokens sequence with an existing transaction, mailbox-row
lock, or conditional write that only persists tokens when the mailbox is not
disconnected. Add a barrier test that pauses after the status read, performs the
disconnect, resumes persistence, and verifies no token row remains.
🪄 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: aa6e60c7-d9dc-430e-9f64-8165bd7681f1
📒 Files selected for processing (9)
specs/mail/gmail-connect.mdsrc/api/gmail-webhook.test.tssrc/mail/gmail-disconnect.test.tssrc/mail/gmail-disconnect.tssrc/mail/gmail-oauth.test.tssrc/mail/gmail-oauth.tssrc/mail/gmail-reconcile.test.tssrc/store/mailboxes.test.tssrc/store/mailboxes.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/mail/gmail-reconcile.test.ts
- src/mail/gmail-disconnect.ts
- src/mail/gmail-disconnect.test.ts
Contributor
Author
Adversarial review + re-gate (orchestrator)Independent gate (re-run from a clean Adversarial review of record: this PR carries two CodeRabbit review passes, not one. Flagging the actual count rather than the briefed one, since they differ:
Net: 1 of 4 actionable findings is resolved; 3 remain open, including a security finding (#1, possible token leakage into logs) and the residual race CodeRabbit itself flagged after reviewing the fix (#4). I did not apply further fixes beyond re-confirming the gate — this comment is reporting status, not closing out the remaining findings. Recommend a human call on whether #1/#3/#4 block merge or are accepted as follow-up (#3 in particular is a five-minute test change with a ready-made diff in CodeRabbit's own comment). 🤖 Generated with Claude Code |
zaridan
added a commit
that referenced
this pull request
Jul 16, 2026
… SQL-fenced refresh guard, rollback test Addresses the three open CodeRabbit findings on PR #46: - revokeToken no longer reads a non-2xx response body into the thrown error at all (the endpoint can echo the submitted refresh token back; a length cap bounds, it does not redact). Error is the status line only. Fixture added where the error body echoes the token, asserting neither the exception nor the disconnect failure logs contain it. - The refresh-vs-disconnect resurrection race is now closed at the SQL layer instead of narrowed in JS: new MailboxTokenStore.upsertTokensUnlessDisconnected runs the status check and token write as ONE guarded INSERT..SELECT..FOR UPDATE..ON CONFLICT statement (zero rows = fence held, nothing written), and the disconnect transaction now flips the mailbox status FIRST so both sides serialize on the mailboxes row lock — same fence discipline as the outbound queue's attempts-generation fence (PR #44). - markDisconnected's tx test now proves rollback PARTICIPATION: a forced transaction rollback takes the status write with it. specs/mail/gmail-connect.md §8 updated to match (§8b step 3 flip-first ordering, §8d race now closed rather than narrowed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
Review round 2 fixes (orchestrator follow-up)Commit cc4a063 addresses the three open CodeRabbit findings:
Gate (this session, worktree clean at cc4a063): 🤖 Generated with Claude Code |
Adds the inverse of HT-40's connect flow: POST /api/v1/inbound/gmail/disconnect (Bearer-gated, ordinary route) revokes the mailbox's OAuth grant at Google (RFC 7009 revoke), stops its Gmail push watch (users.stop), and deactivates it locally. Migration 017 widens mailboxes.status to add 'disconnected'; disconnect deletes the mailbox_oauth_tokens and gmail_watch_state rows in one transaction. Revoke and stop are best-effort (local deactivation always wins per the ticket's explicit ordering decision); disconnecting an already-disconnected mailbox is an idempotent no-op, an unknown address 404s. Adds disconnect coverage to specs/mail/gmail-connect.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ards Address code review findings on the Gmail OAuth disconnect action: - gmail-oauth.ts's refresh() now re-checks the mailbox's current status before persisting a refreshed token, and skips the write when the mailbox is disconnected — closing the race where a concurrent refresh in flight during a disconnect could resurrect a token row for a mailbox an operator just took offline. - gmail-disconnect.ts's already-disconnected idempotent path now still re-runs the step-3 transactional deletes instead of returning a bare no-op, so any resurrected row left by the residual race is cleaned up on a retried disconnect call. - mailboxes.ts's markNeedsReconnect/markPaused now guard with `AND status <> 'disconnected'` so an in-flight pipeline failure can no longer silently overwrite an operator's explicit disconnect; a guarded, existing row is a silent no-op, a genuinely missing row still throws. - Added regression tests pinning that a disconnected mailbox is rejected by the push webhook and skipped by the reconcile handler, mirroring the existing paused-mailbox cases. Updated specs/mail/gmail-connect.md §8d/§8e to describe the revised idempotent-disconnect behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… SQL-fenced refresh guard, rollback test Addresses the three open CodeRabbit findings on PR #46: - revokeToken no longer reads a non-2xx response body into the thrown error at all (the endpoint can echo the submitted refresh token back; a length cap bounds, it does not redact). Error is the status line only. Fixture added where the error body echoes the token, asserting neither the exception nor the disconnect failure logs contain it. - The refresh-vs-disconnect resurrection race is now closed at the SQL layer instead of narrowed in JS: new MailboxTokenStore.upsertTokensUnlessDisconnected runs the status check and token write as ONE guarded INSERT..SELECT..FOR UPDATE..ON CONFLICT statement (zero rows = fence held, nothing written), and the disconnect transaction now flips the mailbox status FIRST so both sides serialize on the mailboxes row lock — same fence discipline as the outbound queue's attempts-generation fence (PR #44). - markDisconnected's tx test now proves rollback PARTICIPATION: a forced transaction rollback takes the status write with it. specs/mail/gmail-connect.md §8 updated to match (§8b step 3 flip-first ordering, §8d race now closed rather than narrowed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zaridan
force-pushed
the
feat/ht-47-gmail-oauth-disconnect
branch
from
July 16, 2026 23:02
cc4a063 to
a7c2bdc
Compare
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.
Summary
Implemented HT-47: the Gmail OAuth disconnect admin action, the inverse of HT-40's connect flow. Added
POST /api/v1/inbound/gmail/disconnect(Bearer-gated, an ordinary route with no pre-auth carve-out) that: (1) revokes the mailbox's stored refresh token at Google's RFC 7009 revoke endpoint via a newrevokeTokenhelper; (2) calls the newGmailWatchClient.stop()(users.stop) to unarm the Gmail push watch, added tosrc/providers/adapters/gmail/watch.tsmirroring its existingwatch/getProfilestyle; (3) deactivates the mailbox locally by marking it'disconnected'(a new 4th lifecycle status added via migration 017, which drops/re-addsmailboxes_status_check) and deleting itsmailbox_oauth_tokensandgmail_watch_staterows in one DB transaction. The orchestration lives in a newsrc/mail/gmail-disconnect.ts(GmailDisconnectService), composed the same waygmail-connect.tsis: injectedcreateWatchClient, injectedfetchImpl, typedGmailDisconnectError. The new route is wired throughsrc/api/gmail-disconnect.ts(JSON body{address}→ JSON result),src/api/router.ts(newGMAIL_DISCONNECTroute),src/api/index.ts(new optionalgmailDisconnectdep, absent-by-default 404s), andsrc/composition/root.ts(constructs and wires the real service). Store layer gainedMailboxStore.markDisconnected,MailboxTokenStore.deleteTokens, andGmailWatchStateStore.deleteState, all accepting an optionaltxfor the atomic disconnect transaction, matching the existingupsertConnectedMailbox/upsertTokens/seedBaselinetx pattern. Added a disconnect section (§8) to specs/mail/gmail-connect.md documenting the route, the best-effort ordering decision, thedisconnectedstatus, and idempotency. Tests cover: revoke/stop adapter behavior (fake fetch, no real network), the full service (real PGlite stores, faked Google) for happy path, idempotent repeat, revoke failure, stop failure, both failing, paused/needs_reconnect mailboxes, no-stored-tokens edge case, and never-leaks-the-token; API handler tests for body validation and error-code mapping; router tests for the new route; wiring tests insrc/api/index.test.tsandsrc/composition/root.test.tsproving Bearer-gating and dep-absence 404s end-to-end.Design decisions
Key decisions, all recorded in the module doc of src/mail/gmail-disconnect.ts and in specs/mail/gmail-connect.md §8:
{"address": "..."}rather than an internal mailboxId, mirroring howMailboxStore.getMailboxByAddressis already the resolution key elsewhere (the push webhook) and how an operator would actually name a mailbox — this wasn't explicitly specified in the ticket brief so I made this call and documented it in the spec and route doc comments.stop()runs BEFORE revoke (not after), because revoking the refresh token can invalidate access tokens issued under that grant, which would make a post-revokestop()call fail against an already-dead token.revoked/watchStoppedbooleans), and never abort the local deactivation transaction — local state always wins, per the ticket's explicit lean. This makes the disconnect response always 2xx once the mailbox is found (except a genuine DB failure), which I judged correct: the ticket calls for local deactivation to be authoritative, so a "partial success" is still a success from the local system's point of view; the response body'srevoked/watchStoppedflags let an operator know a manual Google-side follow-up may be needed.alreadyDisconnected: truewithrevoked/watchStoppedboth false.Review
0 adversarial findings raised, 0 actionable, all addressed.
Verification
Independent gate: typecheck 0, lint 0, tests 0 (exit codes), clean tree.
Plus implementer evidence:
npm run typecheck(tsc --noEmit -p tsconfig.json): exit 0, no errors — confirmed clean on the final state of the tree.npm run lint(biome check .): exit 0, "Checked 183 files ... No fixes applied" — confirmed clean.npx vitest run src/db/migrate.test.ts→ 21/21 passed.npx vitest run src/providers/adapters/gmail/watch.test.ts src/store/mailboxes.test.ts src/store/mailbox-tokens.test.ts src/store/gmail-watch-state.test.ts→ 74/74 passed (pre-new-tests baseline) then re-run after adding markDisconnected/deleteTokens/deleteState coverage → 66/66 passed for the three store files.npx vitest run src/mail/gmail-disconnect.test.ts→ 17/17 passed.npx vitest run src/api/gmail-disconnect.test.ts→ 9/9 passed.npx vitest run src/api/router.test.ts→ 17/17 passed.npx vitest run src/api/index.test.ts(run standalone, no other vitest process competing) → 101/101 passed, 408s.npx vitest run src/composition/root.test.ts(run standalone) → 9/9 passed, 7.9s — this is the true signal for that file; an earlier attempt run CONCURRENTLY with the index.test.ts background job produced 6 spurious failures, every one abeforeEachPGlite-construction hook timeout (10s) under CPU contention, not a real regression — confirmed by the clean standalone re-run.npm test(the full suite) in the background to get one final combined signal; it did not finish inside this session —ps auxshowed the shared machine was simultaneously running fullvitestsuites for at least two OTHER worktrees (feat-ht-46-attachment-blob-persistence, fix-ht-45-stuck-received-reclaim) at the same time, which is why every heavy PGlite-based run here was slow/contended. The partial log before I had to stop showed one failure, insrc/mail/send.test.ts("ON: the html body ... carries a pixel whose token verifies and binds THIS thread") — a file this ticket never touches (open-tracking pixel/send path, unrelated to Gmail OAuth disconnect); given the demonstrated contention-induced flakiness pattern on this machine, I cannot confirm from this run alone whether that is a genuine pre-existing flake or contention noise, and I did not get a clean full-suite result before being required to conclude. Every file this ticket actually changed has been verified individually and passes; I was not able to complete one uncontended full-reponpm testpass in the time available and am reporting that honestly rather than claiming a result I didn't observe.Link https://resonantiq.atlassian.net/browse/HT-47
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
POST /api/v1/inbound/gmail/disconnect) to stop watch notifications, revoke OAuth refresh tokens, and remove local connection data by email address.disconnected.Bug Fixes