ci(auth): run the Better Auth integration suite in CI - #8381
Merged
BhagyaAmarasinghe merged 10 commits intoJul 1, 2026
Merged
Conversation
…elation Commit 51a770c made TwoFactor one-per-user (User.twoFactor TwoFactor?, @@unique(userId)), but the 2FA integration test still queried the old plural `twoFactors` relation and asserted .toHaveLength(1). It has been broken since that change, unnoticed because the integration suite isn't run in CI. Use the singular relation and assert the row is present.
The integration harness (apps/web/integration + the *.integration.test.ts files) is the only runtime validation of the BA<->Formbricks boundary against a real Postgres + Redis (no DB mocks), but no workflow ran it — `pnpm test` mocks @formbricks/database and e2e is Playwright. Add an integration-tests workflow that stands up Postgres + Valkey services, provisions a fresh formbricks_ba_test (migrate + the two BA-shape ALTERs), and runs `pnpm test:integration`. GitHub Actions service containers aren't reachable via `docker exec`, so global-setup's schema clone can't run in CI. Add a TEST_DB_PROVISIONED escape hatch: when set, global-setup trusts the out-of-band provisioning and skips the docker clone (+ redis flush; a fresh CI Valkey starts empty). Local dev behavior is unchanged when the flag is unset.
Contributor
WalkthroughA new GitHub Actions workflow 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 @.github/workflows/integration-tests.yml:
- Around line 83-95: The Better Auth schema shaping is duplicated between the
workflow step and the integration setup, which can cause CI and local
provisioning to drift. Move the ALTER TABLE patch logic into one shared script
or SQL entry point and call it from both the workflow and the setup code, using
the existing integration setup path (global-setup.ts) and the workflow migration
step as the two callers.
- Around line 72-77: The ENTERPRISE_LICENSE_KEY replacement in the
integration-tests workflow is interpolating a secret directly into a
double-quoted sed command, which can break on special characters or trigger
shell expansion. Update the .env rewrite step in the workflow job that edits
REDIS_URL, DATABASE_URL, and ENTERPRISE_LICENSE_KEY to pass the secret via env:
and use a safer data-only replacement approach in that same step rather than
embedding the secret in the sed expression.
- Around line 53-54: The workflow’s initial checkout step still persists the
GITHUB_TOKEN in local git config even though no later step needs git auth.
Update the actions/checkout usage in this integration test workflow to disable
persisted credentials by setting persist-credentials to false, while keeping the
existing dangerous-git-checkout step 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25f3f351-d87c-4963-ba92-46263ca78b5e
📒 Files selected for processing (3)
.github/workflows/integration-tests.ymlapps/web/integration/global-setup.tsapps/web/modules/auth/lib/auth-two-factor.integration.test.ts
The first CI run failed: all 11 suites errored with "Failed to resolve entry for package @formbricks/logger". After a fresh install the workspace packages aren't built, so their package.json `exports` point at missing dist/. Build web's workspace deps (`--filter=@formbricks/web^...`, deps only — not the Next app) before running the suite, mirroring how e2e.yml builds before its package tests.
- Centralize the Better Auth schema-shape ALTERs into apps/web/integration/ba-test-schema-shape.sql, applied by both global-setup.ts (local) and the workflow (CI, via psql -f) — removes the duplication that could drift CI vs local provisioning. - Drop the ENTERPRISE_LICENSE_KEY sed: the harness calls enterprise-gated flows directly and the suite is green with it empty, so the secret-into-sed interpolation (template-injection / special-char breakage) is gone by removing the need entirely. - Set persist-credentials: false on the initial checkout — no later step needs git auth (dangerous-git-checkout re-authenticates its own checkout).
…e positive The doc comment line ending in "... Account.type as NOT NULL;" tripped SonarQube's commented-out-code rule (S125) — it's prose, not disabled SQL. Reword to plain English (no trailing semicolons / SQL keywords). The ALTER statements are unchanged.
- paths: packages/** (was just database+cache) — the harness transitively imports @formbricks/logger, types, email, …; a change to any of those can affect the BA boundary, so they should trigger the suite too. - add a concurrency group (matches pr.yml) so a new push cancels superseded runs.
12 tasks
CodeQL flagged the prisma-generate invocation: it built a shell command string by interpolating
import.meta.url-derived absolute paths (dbDir, testSchema), so a checkout path with shell
metacharacters could break or inject. Switch execSync(template-string) → execFileSync('pnpm', [args])
which runs without a shell and passes the paths as literal argv. Build-script only; no behavior change.
global-setup applied the shared ba-test-schema-shape.sql via `psql -c "<multi-statement>"`, which runs the good ALTERs but exits 0 even when a later statement errors — so a bad/renamed ALTER would silently leave a half-shaped formbricks_ba_test that still "passes". That defeats the point of centralizing the SQL (CI vs local can't drift) since CI applies the same file with `psql -v ON_ERROR_STOP=1 -f`, which does abort on error. Apply the file the same way CI does locally: pipe it to `psql -f -` over stdin with ON_ERROR_STOP, so a failed statement exits non-zero and execSync throws, aborting the harness loudly. Verified: a deliberately-failing ALTER now stops globalSetup (exit 1) instead of reaching a green 35/35. Also surface the otherwise-silent Redis-flush catch with a console.warn — the skip stays non-fatal, but a genuinely failing flush is no longer invisible.
…ar S4036)
SonarCloud flagged `execFileSync("pnpm", ...)` (S4036): the command was resolved
via $PATH, so a planted `pnpm`/`prisma` on a writable PATH entry could run. Invoke
the Prisma CLI through `process.execPath` (the running Node binary's fixed, absolute
path) + the CLI path resolved from node_modules, so the command is never PATH-looked-up.
Still shell-free (keeps the earlier CodeQL fix). Prisma spawns the schema's custom
generators by bare name, so the child keeps node_modules/.bin on PATH — the fixed,
project-owned .bin dir alongside the resolved prisma package (what `pnpm exec` did).
BhagyaAmarasinghe
requested changes
Jun 30, 2026
Address review on the Better Auth integration workflow: - Add a merge_group trigger (matches pr.yml / docker-build-validation.yml) so the suite runs in the merge queue and can be promoted to a required check that gates merges. merge_group events ignore paths, so it always runs in the queue. - Widen the pull_request path filter to pnpm-lock.yaml, root package.json, pnpm-workspace.yaml, and the shared dangerous-git-checkout action — a lockfile-only Better Auth/Prisma transitive bump or a checkout-action change can alter this job's behavior without touching apps/web or packages/**, which would otherwise skip it.
|
BhagyaAmarasinghe
self-requested a review
July 1, 2026 05:39
BhagyaAmarasinghe
approved these changes
Jul 1, 2026
BhagyaAmarasinghe
merged commit Jul 1, 2026
e733fd3
into
epic/better-auth-migration
14 checks passed
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
2 participants
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.
What does this PR do?
Wires the Better Auth integration suite (
apps/web/integration/*+ the*.integration.test.tsfiles) into CI. That harness is the only runtime validation of the Better Auth ↔ Formbricks boundary — it drives the real Better Auth handler against a real Postgres + Redis (no DB mocks), covering sign-up/in/out, email verification + password reset, 2FA, SSO provisioning, account deletion, the forward-auth proxy session read, and the recent security guards (password-less delete-user + inactive-user sign-in). Until now no workflow ran it:pnpm testmocks@formbricks/databaseand the e2e job is Playwright, so these ~35 tests only ran when someone ran them locally.Changes:
.github/workflows/integration-tests.yml— stands up Postgres (pgvector) + Valkey services, provisions a freshformbricks_ba_test(migrate + the two Better-Auth-shape ALTERs:email_verified→ boolean,Account.typenullable), and runspnpm test:integration. Services / Node / pnpm /.envsetup mirrore2e.yml.global-setup.ts— aTEST_DB_PROVISIONEDescape hatch. The harness normally provisions the test DB viadocker exec <container> pg_dump|psql, which can't run against GitHub Actions service containers (reached overlocalhost, notdocker exec-able by name). WhenTEST_DB_PROVISIONED=1, global-setup trusts the workflow's out-of-band provisioning and skips the docker clone (per-test isolation still comes fromresetDb). Local dev behavior is unchanged when the flag is unset.fix(test): the 2FA integration test was already broken on the epic — it queried the old pluraltwoFactorsrelation and asserted.toHaveLength(1), but51a770cdbmadeTwoFactorone-per-user (User.twoFactor TwoFactor?,@@unique(userId)). It went unnoticed precisely because the suite wasn't in CI. Fixed to the singular relation. (This is the first thing the new CI gate caught.)Base:
epic/better-auth-migration(the harness lives on the epic, notmain, so the workflow has to land where it can run).How should this be tested?
Verified locally against the dev Postgres + Valkey, both provisioning paths:
formbricks_ba_test→ migrated from scratch (confirmedTwoFactor_userId_keyunique index) → the two ALTERs (email_verifiedconfirmedboolean) →TEST_DB_PROVISIONED=1 pnpm test:integration→ 35/35 pass.pnpm test:integration→ global-setup's docker clone still runs → 35/35 pass (no regression to local dev).Checklist
Required
pnpm build(the@formbricks/databasepackage, for the migration runner)console.logs(none added)epic/better-auth-migrationand is branched off its current headAppreciated