fix(invitations): pre-P9 hardening — legacy email normalization + best-effort finalize#3829
Conversation
Edited in place (no timestamp bump): the migration has only ever run on ephemeral CI/per-pid test DBs — devkit has no prod, and Trawl prod first runs it at P9 (#3815). Post-chain binary lookups (signin/forgot/OAuth-link/E9) lowercase their input; un-normalized legacy rows would be unreachable accounts.
…emoval TODO on P9
|
Warning Review limit reached
More reviews will be available in 15 minutes and 16 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR hardens the invitation signup flow and email storage by normalizing mixed-case user emails to lowercase and wrapping invitation finalization operations in graceful error handling, ensuring registration succeeds even when post-signup invitation cleanup fails. ChangesInvitations Hardening & Email CI Unique Index
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3829 +/- ##
==========================================
+ Coverage 91.82% 91.84% +0.01%
==========================================
Files 159 159
Lines 5201 5212 +11
Branches 1655 1659 +4
==========================================
+ Hits 4776 4787 +11
Misses 337 337
Partials 88 88
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the invitations/signup flow ahead of P9 by ensuring legacy mixed-case user emails remain reachable after the codebase’s “always-lowercase lookup” behavior, and by making invitation finalize/release steps best-effort so transient DB issues don’t turn successful account creation into an API error.
Changes:
- Extend the
users-email-ci-unique-indexmigration to lowercase legacy mixed-case emails after the duplicate pre-check and before index work. - Make invitation finalize (and one release site) best-effort in auth signup + OAuth paths, with warning logs instead of failing the request post-create.
- Add/adjust integration tests to cover legacy email normalization and finalize failure resilience; update downstream-facing docs/comments.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/users/tests/users.email.ci.index.integration.tests.js | Adds integration coverage for legacy mixed-case email normalization + duplicate pre-check behavior. |
| modules/users/migrations/20260610120000-users-email-ci-unique-index.js | Adds an (a2) normalization pass to lowercase legacy mixed-case emails before index operations. |
| modules/invitations/tests/invitations.integration.tests.js | Adds integration coverage ensuring finalize failures don’t convert a successful signup into an error. |
| modules/auth/routes/auth.routes.js | Updates TODO guidance for when the legacy /api/auth/invitations* alias can be safely removed downstream. |
| modules/auth/controllers/auth.controller.js | Wraps invitation release/finalize calls in best-effort try/catch and logs warnings on finalize failures. |
| MIGRATIONS.md | Documents that mixed-case single emails are now normalized in-place by the migration (no operator action required). |
![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 `@modules/auth/controllers/auth.controller.js`:
- Around line 559-573: The logger.warn call in the oauth invite finalize block
should log the full error object so stack traces are captured; update the catch
block where oauthInvite is handled to pass finalizeErr (not
finalizeErr?.message) to logger.warn, keeping the same message and metadata
(referencing oauthEligibility.finalize, createdUser._id/createdUser.id and the
userId field) so the full error object is available in logs for debugging.
- Around line 203-218: In the catch block after calling eligibility?.finalize in
the if (invite && !config.sign.up) path, change the logger.warn payload so it
passes the full Error object (finalizeErr) instead of only finalizeErr?.message;
update the logger.warn call in the catch for eligibility.finalize to include
err: finalizeErr (and keep userId as String(user._id || user.id)) so Winston can
capture the stack and structured error details.
In `@modules/auth/routes/auth.routes.js`:
- Around line 12-15: The TODO extending removal from P6 to P9 keeps the core
auth module dependent on the optional invitations module longer than intended;
revert the timeline back to P6 (restore the original target milestone) and
update the comment to include the tracking issue number
(pierreb-devkit/Node#3815), the exact prerequisites (downstream Trawl update and
Vue auth.store repointing of verifyInvite), and an owner/ETA so this
technical-debt coupling is timeboxed; ensure the note explicitly calls out the
two imports and the three alias routes in this file so they can be removed when
the prerequisites are met.
- Around line 12-15: Update the inconsistent TODO block so both references use
the same deprecation target: change the inline phrase "temporary coupling until
P6 removes these routes" to reference P9 to match the TODO header
(TODO(P9,...)), ensuring the comment consistently indicates removal after P9 and
clarifying the conditions (downstream Trawl update and Vue auth.store
`verifyInvite` repointing) in the same sentence; locate the TODO comment and the
subsequent explanatory sentence containing "temporary coupling until P6 removes
these routes" and make the phrasing uniform (both P9) and grammatically
consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f54c4bc-a50a-49e4-be25-6c508514e2aa
📒 Files selected for processing (6)
MIGRATIONS.mdmodules/auth/controllers/auth.controller.jsmodules/auth/routes/auth.routes.jsmodules/invitations/tests/invitations.integration.tests.jsmodules/users/migrations/20260610120000-users-email-ci-unique-index.jsmodules/users/tests/users.email.ci.index.integration.tests.js
…arms Addresses review threads: the capacity-gate release swallow is now logged, both finalize warns include the stack, and the alias-coupling comment is gated on P9. Adds OAuth-finalize-throw + cap-release-throw integration tests covering the 3 codecov-missed catch lines.
Pre-P9 fixes (epic #3808) — Node PR-A
From the 2026-06-11 cross-PR review (plan
infra/docs/superpowers/plans/2026-06-11-pre-p9-fixes-invitations.md). Both this and the Vue PR-B gate P9 (#3815).T1 — 🔴 Migration: normalize legacy mixed-case emails (THE P9 blocker)
Post-chain, every email lookup lowercases its input and a plain
findOneis a binary match — a legacyPierre@Gmail.comrow (possible pre-chain) becomes an unreachable account (signin/forgot/OAuth-link/E9 all miss it). The…120000migration now lowercases mixed-case singles after the dup pre-check (which guarantees no collision) and before the index work. Pipeline$toLower, idempotent.Edited in place (no timestamp bump) — deliberate: the migration has only ever run on ephemeral CI/per-pid test DBs; devkit has no prod; Trawl prod first runs it at P9.
Tests: raw-insert mixed-case row →
up()→ binary lowercase read-back; dup-laden DB still ABORTS with zero rows modified.T2 — Best-effort finalize/release
A finalize DB hiccup after
UserService.createcould 422 an already-created account (user exists, can sign in; invite zombies until the sweep). Both finalize sites (signup + OAuth) now try/catch +logger.warn; the one unwrapped release mirrors the other 3. Test: mockedInvitationsService.acceptrejection → signup still 200.T3 — Doc fixes
2 stale "case-sensitive index" comments (P2-era, contradicted the P3 collation index) + the alias-removal TODO re-gated on P9 + the Vue verifyInvite repoint (executing the old TODO would have silently killed invite links — the Vue store still called the alias).
Verification
Lint clean; touched suites green (5/5 migration, 29/29 invitations integration); plan-anchored TDD (failing-first evidence per task); spec-reviewed against the plan.
Part of #3808. Pairs with Vue PR-B.
Summary by CodeRabbit
Bug Fixes
Chores