feat(sso): SAML/OIDC single sign-on#3911
Open
0ski wants to merge 1 commit into
Open
Conversation
Vendor-neutral plugin contract plus the host wiring that consumes it. With no SSO plugin installed, everything degrades to a no-op fallback, so OSS deployments are unaffected. - Plugin contract (@trigger.dev/plugins) + lazy loader/fallback in internal-packages/sso: status, portal-link, enforce/JIT config, route-decision, begin/complete authorization, identity resolution, JIT evaluation, and periodic session validation. All methods return neverthrow Results; the fallback is fail-open. - Login: 'Sign in with SSO' entry + dedicated /login/sso flow and /auth/sso(.callback) routes, plus auto-discovery from magic-link/OAuth. - Org settings -> SSO page: plan-tier upsell, connection status, verified-domain list, enforcement + JIT provisioning + default-role configuration, and an admin-portal link dialog. - AuthUser carries an optional signed 'sso' marker; SSO-established sessions are periodically re-validated against the identity provider on a single-flight, throttled, fail-open basis and logged out only on an explicit invalid result. - SSO_ENABLED gate (default off) so the feature ships dark until its backing plugin is available; SSO_SESSION_REVALIDATION_INTERVAL_SECONDS controls the cadence.
🦋 Changeset detectedLatest commit: 6bc31eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThis pull request introduces vendor-neutral SSO (Single Sign-On) support to Trigger.dev. It defines a public 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/plugins
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+48
to
+51
| const auth = await authenticator.authenticate("sso", request, { | ||
| throwOnError: true, | ||
| context: { profile, flow }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🔴 Missing try/catch around authenticator.authenticate with throwOnError: true in SSO callback
The SSO callback route calls authenticator.authenticate("sso", request, { throwOnError: true }) without a try/catch block. With throwOnError: true, remix-auth's Strategy.failure() throws an AuthorizationError instead of redirecting. The SsoStrategy.authenticate() method (apps/webapp/app/services/ssoAuth.server.ts:47-53) catches verify-callback errors and calls this.failure(), which propagates as an unhandled AuthorizationError — resulting in a 500 error page.
The verify callback can fail for multiple legitimate reasons: resolveSsoIdentity returning an error (ssoAuth.server.ts:61-65), DB errors during findOrCreateSsoUser, or transient failures in ensureOrgMember. This same PR correctly wraps the identical authenticator.authenticate(..., { throwOnError: true }) call in try/catch blocks in both the GitHub callback (apps/webapp/app/routes/auth.github.callback.tsx:26-39) and Google callback (apps/webapp/app/routes/auth.google.callback.tsx:26-39), but the SSO callback omits it.
Suggested change
| const auth = await authenticator.authenticate("sso", request, { | |
| throwOnError: true, | |
| context: { profile, flow }, | |
| }); | |
| let auth; | |
| try { | |
| auth = await authenticator.authenticate("sso", request, { | |
| throwOnError: true, | |
| context: { profile, flow }, | |
| }); | |
| } catch (thrown) { | |
| if (thrown instanceof Response) throw thrown; | |
| logger.warn("SSO authentication failed", { error: thrown }); | |
| return redirect("/login/sso?error=sso_failed"); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/services/session.server.ts (1)
64-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun SSO revalidation in the impersonation branch too.
The impersonation path returns before the new revalidation call, so SSO sessions can bypass IdP invalidation checks while impersonating.
Suggested fix
if (impersonatedUserId) { @@ const authUser = await authenticator.isAuthenticated(request); if (!authUser?.userId) return undefined; + await revalidateSsoSession(request, authUser); const realUser = await getUserById(authUser.userId);
🧹 Nitpick comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsx (1)
204-218: 💤 Low valueConsider wrapping SSO config writes in a transaction for atomicity.
The three parallel writes can leave partial state if one fails while others succeed. While the comment documents this behavior, wrapping them in a
prisma.$transaction(or havingssoControllerexpose a singleupdateConfigmethod) would ensure all-or-nothing semantics for a cleaner UX.internal-packages/sso/src/index.ts (1)
214-224: 💤 Low valueConsider using a named export instead of default export.
The codebase guideline prefers named exports over default exports for
*.{ts,tsx,js,jsx}files. Consider refactoring to a named function export pattern.♻️ Suggested refactor
-class Sso { - // Synchronous — returns a lazy controller that resolves any installed - // plugin on first call. - create(prisma: SsoPrismaInput, options?: SsoCreateOptions): SsoController { - return new LazyController(prisma, options); - } -} - -const loader = new Sso(); - -export default loader; +// Synchronous — returns a lazy controller that resolves any installed +// plugin on first call. +export function createSsoController( + prisma: SsoPrismaInput, + options?: SsoCreateOptions +): SsoController { + return new LazyController(prisma, options); +}Then update the calling site in
apps/webapp/app/services/sso.server.ts:-import sso from "`@trigger.dev/sso`"; +import { createSsoController } from "`@trigger.dev/sso`"; -export const ssoController = sso.create( +export const ssoController = createSsoController(Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2c79bb72-b10e-41bf-ac52-d2a63e492e3d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (47)
.changeset/sso-plugin-contract.md.server-changes/accounts-webhook-passthrough.md.server-changes/sso-plugin-plumbing.mdapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/env.server.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/models/user.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/login._index/route.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/services/authUser.tsapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/v3/accountsWebhookWorker.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/package.jsoninternal-packages/database/prisma/migrations/20260527130000_add_sso_authentication_method/migration.sqlinternal-packages/database/prisma/schema.prismainternal-packages/sso/package.jsoninternal-packages/sso/src/fallback.tsinternal-packages/sso/src/index.tsinternal-packages/sso/src/loader.test.tsinternal-packages/sso/tsconfig.jsoninternal-packages/sso/vitest.config.tspackages/plugins/package.jsonpackages/plugins/src/index.tspackages/plugins/src/sso.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (41)
- GitHub Check: audit
- GitHub Check: audit
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (10, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (12, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (8, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (11, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (6, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (7, 12)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: internal / 🧪 Unit Tests: Internal (4, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (3, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (9, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1, 12)
- GitHub Check: internal / 🧪 Unit Tests: Internal (5, 12)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: sdk-compat / Node.js 20.20 (ubuntu-latest)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: typecheck / typecheck
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build and publish previews
- GitHub Check: 🛡️ E2E Auth Tests (full)
🧰 Additional context used
📓 Path-based instructions (14)
internal-packages/database/**/prisma/migrations/*/*.sql
📄 CodeRabbit inference engine (internal-packages/database/CLAUDE.md)
internal-packages/database/**/prisma/migrations/*/*.sql: Clean up generated Prisma migrations by removing extraneous lines for junction tables (_BackgroundWorkerToBackgroundWorkerFile,_BackgroundWorkerToTaskQueue,_TaskRunToTaskRunTag,_WaitpointRunConnections,_completedWaitpoints) and indexes (SecretStore_key_idx, variousTaskRunindexes) unless explicitly added
When adding indexes to existing tables, useCREATE INDEX CONCURRENTLY IF NOT EXISTSto avoid table locks in production, and place each concurrent index in its own separate migration file
Indexes on newly created tables can useCREATE INDEXwithout CONCURRENTLY and can be combined in the same migration file as theCREATE TABLEstatement
When adding an index on a new column in an existing table, use two separate migrations: first forALTER TABLE ... ADD COLUMN IF NOT EXISTS ..., then forCREATE INDEX CONCURRENTLY IF NOT EXISTS ...in its own file
Files:
internal-packages/database/prisma/migrations/20260527130000_add_sso_authentication_method/migration.sql
**/*.{js,ts,tsx,jsx,css,json,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier for code formatting and run
pnpm run formatbefore committing
Files:
packages/plugins/package.jsonapps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsinternal-packages/sso/package.jsonapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxinternal-packages/sso/tsconfig.jsonapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/package.jsonapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects insteadImport from
@trigger.dev/sdkwhen writing Trigger.dev tasks. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob
Files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/services/authUser.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when circular dependencies cannot be resolved, code splitting is needed for performance, or the module must be loaded conditionally at runtime
Import subpaths only frompackages/core(@trigger.dev/core), never import from the root
Files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/services/ssoSessionRevalidation.server.tspackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.tspackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/models/user.server.tsapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsinternal-packages/sso/src/index.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathUse named constants for sentinel/placeholder values (e.g.
const UNSET_VALUE = '__unset__') instead of raw string literals scattered across comparisons
Files:
apps/webapp/app/services/authUser.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
apps/webapp/**/*.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/**/*.server.ts: Never userequest.signalfor detecting client disconnects. UsegetRequestAbortSignal()fromapp/services/httpAsyncStorage.server.tsinstead, which is wired directly to Expressres.on('close')and fires reliably
Access environment variables viaenvexport fromapp/env.server.ts. Never useprocess.envdirectly
Always usefindFirstinstead offindUniquein Prisma queries.findUniquehas an implicit DataLoader that batches concurrent calls and has active bugs even in Prisma 6.x (uppercase UUIDs returning null, composite key SQL correctness issues, 5-10x worse performance).findFirstis never batched and avoids this entire class of issues
Files:
apps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/models/user.server.tsapps/webapp/app/v3/accountsWebhookWorker.server.ts
apps/webapp/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Only use
useCallback/useMemofor context provider values, expensive derived data that is a dependency elsewhere, or stable refs required by a dependency array. Don't wrap ordinary event handlers or trivial computations
Files:
apps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
**/tsconfig.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use strict mode in TypeScript configuration
Files:
internal-packages/sso/tsconfig.json
apps/webapp/app/v3/**Worker.server.ts
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Do NOT add new jobs using zodworker/graphile-worker (legacy). Background job workers use
@trigger.dev/redis-workervia files likeapp/v3/commonWorker.server.ts,app/v3/alertsWorker.server.ts,app/v3/batchTriggerWorker.server.ts
Files:
apps/webapp/app/v3/accountsWebhookWorker.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
internal-packages/sso/src/loader.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Never mock anything in tests - use testcontainers instead
Test files should be placed next to source files (e.g.,MyService.ts->MyService.test.ts)
Files:
internal-packages/sso/src/loader.test.ts
**/*.test.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{js,ts,tsx}: Test files should live beside the files under test and use descriptivedescribeanditblocks
Use vitest for unit testing
Tests should avoid mocks or stubs and use helpers from@internal/testcontainerswhen Redis or Postgres are needed
Files:
internal-packages/sso/src/loader.test.ts
🧠 Learnings (27)
📚 Learning: 2026-02-03T18:48:31.790Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: internal-packages/database/prisma/migrations/20260129162810_add_integration_deployment/migration.sql:14-18
Timestamp: 2026-02-03T18:48:31.790Z
Learning: For Prisma migrations targeting PostgreSQL: - When adding indexes to existing tables, create the index in a separate migration file and include CONCURRENTLY to avoid locking the table. - For indexes on newly created tables (in CREATE TABLE statements), you can create the index in the same migration file without CONCURRENTLY. This reduces rollout complexity for new objects while protecting uptime for existing structures.
Applied to files:
internal-packages/database/prisma/migrations/20260527130000_add_sso_authentication_method/migration.sql
📚 Learning: 2026-03-22T13:49:20.068Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: internal-packages/database/prisma/migrations/20260318114244_add_prompt_friendly_id/migration.sql:5-5
Timestamp: 2026-03-22T13:49:20.068Z
Learning: For Prisma migration SQL files under `internal-packages/database/prisma/migrations/`, it is acceptable to create indexes with `CREATE INDEX` / `CREATE UNIQUE INDEX` (i.e., without `CONCURRENTLY`) when the parent table is introduced in the same PR and has no existing production rows yet. Only require `CREATE INDEX CONCURRENTLY` (or otherwise account for existing production data/locks) when the table already exists in production with data.
Applied to files:
internal-packages/database/prisma/migrations/20260527130000_add_sso_authentication_method/migration.sql
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxpackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxpackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxinternal-packages/sso/src/index.tsapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/services/authUser.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.sso.tsapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/models/user.server.tsapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/v3/accountsWebhookWorker.server.tsapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-06-04T18:16:35.386Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3836
File: apps/supervisor/src/backpressure/backpressureMonitor.ts:3-5
Timestamp: 2026-06-04T18:16:35.386Z
Learning: When reviewing TypeScript in this repo, apply the rule “prefer type aliases over interfaces” only to data/object shapes and union/intersection type modeling. If an interface is being used as a behavioral contract for collaborators to implement (e.g., method-shape interfaces that define required behavior, such as `BackpressureLogger` / `BackpressureSignalSource` in `apps/supervisor/src/backpressure/backpressureMonitor.ts`), keep it as an `interface` and do not flag it as a type-alias-vs-interface violation.
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/services/ssoSessionRevalidation.server.tspackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.tspackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/models/user.server.tsapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsinternal-packages/sso/src/index.ts
📚 Learning: 2026-06-09T17:58:04.699Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3879
File: apps/webapp/app/models/vercelIntegration.server.ts:619-630
Timestamp: 2026-06-09T17:58:04.699Z
Learning: In this codebase, outbound raw `fetch` calls should typically rely on Node/undici’s default request timeout (about ~300s) rather than adding a per-call `AbortController` + `setTimeout` wrapper inside individual functions (e.g. in files like `apps/webapp/app/models/vercelIntegration.server.ts`). During code review, do not flag the absence of a per-call timeout on a single `fetch` as an issue; if per-call timeouts are needed, they should be implemented via a codebase-wide convention (e.g., a shared fetch wrapper or documented pattern) rather than ad-hoc per-function changes.
Applied to files:
apps/webapp/app/services/authUser.tsinternal-packages/sso/vitest.config.tsapps/webapp/app/utils/pathBuilder.tsapps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/v3/featureFlags.tsinternal-packages/sso/src/fallback.tsapps/webapp/app/routes/webhooks.v1.accounts.tsapps/webapp/app/services/ssoSessionRevalidation.server.tspackages/plugins/src/index.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.tspackages/plugins/src/sso.tsapps/webapp/app/routes/auth.sso.tsapps/webapp/app/models/user.server.tsapps/webapp/app/v3/accountsWebhookWorker.server.tsinternal-packages/sso/src/loader.test.tsinternal-packages/sso/src/index.ts
📚 Learning: 2026-05-01T15:45:08.099Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3499
File: packages/plugins/tsup.config.ts:3-3
Timestamp: 2026-05-01T15:45:08.099Z
Learning: In build/tool configuration files (e.g., tsup.config.ts, vite.config.ts, vitest.config.ts), follow the tool’s documented export pattern and use `export default defineConfig(...)` (or the equivalent documented default export). The repo-wide guideline “use named exports instead of default exports” should apply only to application code (*.{ts,tsx,js,jsx}), not to these build/tool config files—so do not flag `export default defineConfig(...)` in these config files as a violation.
Applied to files:
internal-packages/sso/vitest.config.ts
📚 Learning: 2026-04-30T21:28:35.705Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3473
File: internal-packages/database/prisma/schema.prisma:59-60
Timestamp: 2026-04-30T21:28:35.705Z
Learning: When reviewing Prisma schema files in this repository, do not suggest using Prisma’s `@check` model/table-level attribute or any native Prisma schema syntax for CHECK constraints. Prisma does not implement CHECK constraints (see prisma/prisma#3388). If a CHECK constraint is required, add it only via raw SQL in a handwritten migration (e.g., `ALTER TABLE ... ADD CONSTRAINT ... CHECK (...)`).
Applied to files:
internal-packages/database/prisma/schema.prisma
📚 Learning: 2026-03-26T09:02:07.973Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3274
File: apps/webapp/app/services/runsReplicationService.server.ts:922-924
Timestamp: 2026-03-26T09:02:07.973Z
Learning: When parsing Trigger.dev task run annotations in server-side services, keep `TaskRun.annotations` strictly conforming to the `RunAnnotations` schema from `trigger.dev/core/v3`. If the code already uses `RunAnnotations.safeParse` (e.g., in a `#parseAnnotations` helper), treat that as intentional/necessary for atomic, schema-accurate annotation handling. Do not recommend relaxing the annotation payload schema or using a permissive “passthrough” parse path, since the annotations are expected to be written atomically in one operation and should not contain partial/legacy payloads that would require a looser parser.
Applied to files:
apps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.ts
📚 Learning: 2026-05-05T09:38:02.512Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3523
File: apps/webapp/app/routes/api.v3.batches.ts:178-181
Timestamp: 2026-05-05T09:38:02.512Z
Learning: When reviewing code that catches `ServiceValidationError` in `*.server.ts` files, do not blindly forward `error.status` to HTTP responses, because SVEs may be thrown with non-default statuses (e.g., 400/500) and forwarding them can cause client-visible behavioral regressions (e.g., surfacing 500s to clients). Prefer a safe default response status of `error.status ?? 422`, but only after confirming via the reachable call graph that the caught `ServiceValidationError` instances are expected to carry those non-default statuses; otherwise, normalize to `422` to avoid unexpected client-visible 5xx behavior.
Applied to files:
apps/webapp/app/services/lastAuthMethod.server.tsapps/webapp/app/services/sso.server.tsapps/webapp/app/services/gitHubAuth.server.tsapps/webapp/app/services/ssoRateLimiter.server.tsapps/webapp/app/services/auth.server.tsapps/webapp/app/services/ssoSessionRevalidation.server.tsapps/webapp/app/models/orgMember.server.tsapps/webapp/app/env.server.tsapps/webapp/app/services/session.server.tsapps/webapp/app/services/ssoAuth.server.tsapps/webapp/app/services/ssoAutoDiscovery.server.tsapps/webapp/app/services/googleAuth.server.tsapps/webapp/app/models/user.server.tsapps/webapp/app/v3/accountsWebhookWorker.server.ts
📚 Learning: 2026-05-14T14:54:39.095Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3545
File: .server-changes/agent-view-sessions.md:10-10
Timestamp: 2026-05-14T14:54:39.095Z
Learning: In the `trigger.dev` repository, do not flag inconsistent dot vs slash notation in route/path strings inside `.server-changes/*.md` files. These markdown files are consumed verbatim into the changelog, so the mixed notation (e.g., `resources.orgs.../runs.$runParam/...`) is intentional and should be preserved as-is.
Applied to files:
.server-changes/accounts-webhook-passthrough.md.server-changes/sso-plugin-plumbing.md
📚 Learning: 2026-03-29T19:16:28.864Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3291
File: apps/webapp/app/v3/featureFlags.ts:53-65
Timestamp: 2026-03-29T19:16:28.864Z
Learning: When reviewing TypeScript code that uses Zod v3, treat `z.coerce.*()` schemas as their direct Zod type (e.g., `z.coerce.boolean()` returns a `ZodBoolean` with `_def.typeName === "ZodBoolean"`) rather than a `ZodEffects`. Only `.preprocess()`, `.refine()`/`.superRefine()`, and `.transform()` are expected to wrap schemas in `ZodEffects`. Therefore, in reviewers’ logic like `getFlagControlType`, do not flag/unblock failures that require unwrapping `ZodEffects` when the input schema is a `z.coerce.*` schema.
Applied to files:
apps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/accountsWebhookWorker.server.ts
📚 Learning: 2026-06-09T16:27:26.195Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3878
File: apps/webapp/app/v3/services/computeTemplateCreation.server.ts:0-0
Timestamp: 2026-06-09T16:27:26.195Z
Learning: When working in triggerdotdev/trigger.dev code related to worker-group/region default resolution (e.g., defaultWorkerInstanceGroupId handling used by getGlobalDefaultWorkerGroup, getDefaultWorkerGroupForProject, and RegionsPresenter), do NOT add org-level featureFlags overrides in only one resolution site. That can cause template creation routing/decisions to diverge from actual run routing. If org-level override of the default region/worker group is required, it must be centralized in getGlobalDefaultWorkerGroup so every resolution path remains aligned.
Applied to files:
apps/webapp/app/v3/featureFlags.tsapps/webapp/app/v3/accountsWebhookWorker.server.ts
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-08T21:00:20.973Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3538
File: apps/webapp/app/components/primitives/Resizable.tsx:60-78
Timestamp: 2026-05-08T21:00:20.973Z
Learning: In the triggerdotdev/trigger.dev codebase, treat Zod as a boundary validation tool (API handlers, request/response validation, and storage/DB read/write validation), not as inline render-time validation inside React components/primitive UI code. For render-time guards, prefer small manual type-narrowing checks (e.g., a short predicate like ~10–20 lines) over importing Zod into UI primitives, to avoid per-render schema-parse overhead and unnecessary abstraction. Use the manual guard approach unless you truly need schema validation at a boundary; only then introduce Zod.
Applied to files:
apps/webapp/app/routes/vercel.onboarding.tsxapps/webapp/app/services/emailAuth.server.tsxapps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/auth.google.callback.tsxapps/webapp/app/routes/magic.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/routes/auth.github.callback.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/auth.sso.callback.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-04-02T19:18:26.255Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3319
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions/route.tsx:179-189
Timestamp: 2026-04-02T19:18:26.255Z
Learning: In this repo’s route components that render the Inspector `ResizablePanelGroup` panels, it’s acceptable to pass `collapsed={!isShowingInspector}` together with a no-op `onCollapseChange={() => {}}` when panel visibility is intentionally controlled only by route parameters (e.g., `*Param` search/route params) rather than user drag/collapse interactions. Do not flag an empty/no-op `onCollapseChange` as “missing wiring” in these cases; only flag it when collapse state is expected to change based on user interaction.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-12T21:04:00.184Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.sessions._index/route.tsx:40-42
Timestamp: 2026-05-12T21:04:00.184Z
Learning: In triggerdotdev/trigger.dev route loader implementations (Remix `route.tsx` files under `apps/webapp/app/routes/**`), follow the existing convention for missing/unauthorized environment lookups: when `findEnvironmentBySlug` (or the equivalent env resolver) returns a falsy value, handle it by throwing `new Error("Environment not found")` rather than returning a `404` `Response` (i.e., do not flag this as “missing 404 response”). Changing the error-to-404 convention is a cross-cutting refactor and should be left out of individual PRs unless the PR explicitly addresses that broader migration.
Applied to files:
apps/webapp/app/routes/login.mfa/route.tsxapps/webapp/app/routes/login.sso/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings/route.tsxapps/webapp/app/routes/login.magic/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings.sso/route.tsxapps/webapp/app/routes/login._index/route.tsx
📚 Learning: 2026-05-20T17:21:18.543Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3678
File: apps/webapp/app/entry.server.tsx:0-0
Timestamp: 2026-05-20T17:21:18.543Z
Learning: In env.server.ts (Zod env schema), any environment variable you plan to access via the typed `env` export (e.g., `env.SENTRY_DSN`) must be explicitly declared in the schema. For `SENTRY_DSN`, include `SENTRY_DSN: z.string().optional()`; otherwise switching from `process.env.SENTRY_DSN` to `env.SENTRY_DSN` will fail TypeScript typechecking.
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2026-06-01T11:37:08.569Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3754
File: apps/webapp/app/env.server.ts:1104-1129
Timestamp: 2026-06-01T11:37:08.569Z
Learning: In apps/*/app/env.server.ts, any new background/periodic worker feature flag should hard-default to "0" (explicit opt-in) rather than inheriting from a parent flag (e.g., avoid defaulting to process.env.TRIGGER_MOLLIFIER_ENABLED ?? "0"). Inheriting can cause the new worker to auto-start on upgrade for deployments that already enabled the parent flag, turning on unexpected background load without an explicit rollout. Each worker component must require its own dedicated env var and default it explicitly to "0" (e.g., TRIGGER_MOLLIFIER_STALE_SWEEP_ENABLED defaults to "0" unless explicitly set to enable that worker).
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2026-04-16T14:21:15.229Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/components/logs/LogsTaskFilter.tsx:135-163
Timestamp: 2026-04-16T14:21:15.229Z
Learning: When rendering lists of task registry items in apps/webapp (e.g., <SelectItem /> rows) and using `key={item.slug}`, do not flag it as potentially non-unique. In trigger.dev’s `TaskIdentifier` table, the DB constraint `@unique([runtimeEnvironmentId, slug])` guarantees `slug` is unique within a given runtime environment, so `item.slug` is safe as the React key as long as the list is derived from that registry/constraint (and not from a legacy query that could produce duplicate slugs).
Applied to files:
apps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsx
📚 Learning: 2026-04-27T16:46:03.861Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 3456
File: apps/webapp/package.json:152-152
Timestamp: 2026-04-27T16:46:03.861Z
Learning: In `apps/webapp/package.json`, treat the `effect` npm package as an intentional runtime dependency (not unused/misplaced) for the Schedule + Fiber-based metadata update logic. This should apply when reviewing `apps/webapp` code paths used by `apps/webapp/app/utils/updateMetadata.server.ts` (and closely related modules) that use Effect APIs such as `Duration.divide`, `STM.cond`, namespace exports for `Effect`/`Schedule`/`Duration`/`Fiber`, and the `Fiber.RuntimeFiber` type.
Applied to files:
apps/webapp/package.json
📚 Learning: 2026-05-14T08:21:07.614Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3614
File: apps/webapp/app/v3/mollifier/mollifierGate.server.ts:48-52
Timestamp: 2026-05-14T08:21:07.614Z
Learning: When using Trigger.dev v3 feature flags in the webapp, prefer the existing per-org gating mechanism supported by `flag()` via the `overrides` argument. Pass `Organization.featureFlags` (from `environment.organization.featureFlags`) as the `overrides` value; overrides must take precedence over the global `featureFlag` row. Do not require schema changes or add an `orgId` field to `FlagsOptions` for per-org gating—use the overrides pattern consistently (e.g., in gate flows like `resolveOrgFlag` and any server code that threads `environment.organization.featureFlags` into the gate call).
Applied to files:
apps/webapp/app/v3/accountsWebhookWorker.server.ts
📚 Learning: 2026-05-18T14:40:02.173Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3658
File: packages/core/src/v3/realtimeStreams/manager.test.ts:1-147
Timestamp: 2026-05-18T14:40:02.173Z
Learning: In the triggerdotdev/trigger.dev repo, the policy “Never mock anything — use testcontainers instead” should only be enforced for integration tests that interact with real external services (e.g., Redis, Postgres) via actual infrastructure. For unit tests that exercise pure in-memory logic (e.g., cache semantics) it is OK to stub collaborators such as `ApiClient` using Vitest (`vi.fn()`) to assert call counts or control behavior. Do not flag `vi.fn()`-based `ApiClient` stubs in unit tests as violations of the testcontainers policy.
Applied to files:
internal-packages/sso/src/loader.test.ts
🪛 LanguageTool
.server-changes/sso-plugin-plumbing.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...in loader (@trigger.dev/sso) into the webapp: SSO auth method, hasSso flag, `SsoStr...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
Comment on lines
+31
to
+46
| const existing = await prisma.orgMember.findFirst({ | ||
| where: { userId, organizationId }, | ||
| select: { id: true }, | ||
| }); | ||
| if (existing) { | ||
| return { created: false, orgMemberId: existing.id }; | ||
| } | ||
|
|
||
| const member = await prisma.orgMember.create({ | ||
| data: { | ||
| userId, | ||
| organizationId, | ||
| role: "MEMBER", | ||
| }, | ||
| select: { id: true }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Make ensureOrgMember truly idempotent under concurrency.
The findFirst → create sequence races: two concurrent requests can both miss on Line 31 and one will throw a unique-constraint error on Line 39, which breaks sign-in/JIT provisioning instead of returning { created: false }.
Suggested fix
+import { Prisma } from "`@prisma/client`";
import { prisma } from "~/db.server";
import { logger } from "~/services/logger.server";
import { rbac } from "~/services/rbac.server";
@@
- const member = await prisma.orgMember.create({
- data: {
- userId,
- organizationId,
- role: "MEMBER",
- },
- select: { id: true },
- });
+ let member: { id: string };
+ try {
+ member = await prisma.orgMember.create({
+ data: {
+ userId,
+ organizationId,
+ role: "MEMBER",
+ },
+ select: { id: true },
+ });
+ } catch (error) {
+ if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === "P2002") {
+ const existingAfterConflict = await prisma.orgMember.findFirst({
+ where: { userId, organizationId },
+ select: { id: true },
+ });
+ if (existingAfterConflict) {
+ return { created: false, orgMemberId: existingAfterConflict.id };
+ }
+ }
+ throw error;
+ }
Comment on lines
+330
to
+333
| assertEmailAllowed(email); | ||
|
|
||
| const normalised = email.toLowerCase().trim(); | ||
| const existingUser = await prisma.user.findFirst({ where: { email: normalised } }); |
Contributor
There was a problem hiding this comment.
Validate the canonicalized email value before policy checks.
Line 330 validates raw email, but Lines 332/348 use normalised for lookup/write. Validate the same canonical value you persist, otherwise case/whitespace variants can bypass or misapply assertEmailAllowed.
Suggested fix
export async function findOrCreateSsoUser({
email,
firstName,
lastName,
}: FindOrCreateSso): Promise<LoggedInUser> {
- assertEmailAllowed(email);
-
const normalised = email.toLowerCase().trim();
+ assertEmailAllowed(normalised);
const existingUser = await prisma.user.findFirst({ where: { email: normalised } });Also applies to: 348-349
Comment on lines
+48
to
+51
| const auth = await authenticator.authenticate("sso", request, { | ||
| throwOnError: true, | ||
| context: { profile, flow }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing error handling around authenticate call.
Unlike the GitHub/Google/magic callbacks (which wrap authenticator.authenticate in try-catch), this callback doesn't handle errors thrown by the SSO strategy's verify callback. If the find-or-create-user logic fails (DB errors, constraint violations, etc.), the error propagates unhandled instead of redirecting gracefully to /login/sso?error=sso_failed.
🛠️ Suggested fix
- const auth = await authenticator.authenticate("sso", request, {
- throwOnError: true,
- context: { profile, flow },
- });
+ let auth: Awaited<ReturnType<typeof authenticator.authenticate>>;
+ try {
+ auth = await authenticator.authenticate("sso", request, {
+ throwOnError: true,
+ context: { profile, flow },
+ });
+ } catch (thrown) {
+ if (thrown instanceof Response) throw thrown;
+ logger.error("SSO authenticate failed", { error: thrown });
+ return redirect(`/login/sso?error=sso_failed`);
+ }| }); | ||
| } | ||
|
|
||
| return { userId }; |
Contributor
There was a problem hiding this comment.
Persist SSO context in the authenticated session payload.
On Line 131, returning only { userId } leaves AuthUser.sso unset, so revalidateSsoSession() will skip these SSO sessions entirely.
Suggested fix
- return { userId };
+ return {
+ userId,
+ sso: {
+ idpOrgId: profile.idpOrgId,
+ connectionId: flow.connectionId,
+ },
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { userId }; | |
| return { | |
| userId, | |
| sso: { | |
| idpOrgId: profile.idpOrgId, | |
| connectionId: flow.connectionId, | |
| }, | |
| }; |
Comment on lines
+21
to
+25
| const decision = await ssoController.decideRouteForEmail(normalised); | ||
| if (decision.isErr()) { | ||
| logger.warn("SSO auto-discovery fail-open", { reason: decision.error, email: normalised }); | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Fail-open behavior is incomplete when decideRouteForEmail() throws.
Lines 22-25 only handle Result errors. If ssoController.decideRouteForEmail() throws, login flow fails closed instead of fail-open.
Suggested fix
- const decision = await ssoController.decideRouteForEmail(normalised);
+ let decision;
+ try {
+ decision = await ssoController.decideRouteForEmail(normalised);
+ } catch (error) {
+ logger.warn("SSO auto-discovery fail-open", { reason: error, email: normalised });
+ return null;
+ }
if (decision.isErr()) {
logger.warn("SSO auto-discovery fail-open", { reason: decision.error, email: normalised });
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const decision = await ssoController.decideRouteForEmail(normalised); | |
| if (decision.isErr()) { | |
| logger.warn("SSO auto-discovery fail-open", { reason: decision.error, email: normalised }); | |
| return null; | |
| } | |
| let decision; | |
| try { | |
| decision = await ssoController.decideRouteForEmail(normalised); | |
| } catch (error) { | |
| logger.warn("SSO auto-discovery fail-open", { reason: error, email: normalised }); | |
| return null; | |
| } | |
| if (decision.isErr()) { | |
| logger.warn("SSO auto-discovery fail-open", { reason: decision.error, email: normalised }); | |
| return null; | |
| } |
Comment on lines
+52
to
+53
| const retryAfter = new Date(result.reset).getTime() - Date.now(); | ||
| throw new SsoRateLimitError(retryAfter); |
Contributor
There was a problem hiding this comment.
Clamp retryAfter to a non-negative value.
Line 52 and Line 60 can produce negative durations. Clamp to >= 0 before throwing.
Suggested fix
- const retryAfter = new Date(result.reset).getTime() - Date.now();
+ const retryAfter = Math.max(0, new Date(result.reset).getTime() - Date.now());
throw new SsoRateLimitError(retryAfter);Also applies to: 60-61
Comment on lines
+80
to
+93
| const result = await Promise.race([ | ||
| // ResultAsync is a PromiseLike; Promise.resolve unwraps it to a Result. | ||
| Promise.resolve( | ||
| ssoController.validateSession({ | ||
| userId: authUser.userId, | ||
| idpOrgId: authUser.sso.idpOrgId, | ||
| connectionId: authUser.sso.connectionId, | ||
| }) | ||
| ), | ||
| new Promise<typeof REVALIDATION_TIMEOUT>((resolve) => { | ||
| timer = setTimeout(() => resolve(REVALIDATION_TIMEOUT), timeoutMs); | ||
| }), | ||
| ]); | ||
| if (timer) clearTimeout(timer); |
Contributor
There was a problem hiding this comment.
Guard Promise.race with fail-open error handling.
If ssoController.validateSession(...) rejects/throws, this path currently bubbles an exception instead of failing open, which contradicts the intended behavior.
Suggested fix
const timeoutMs = env.SSO_SESSION_REVALIDATION_TIMEOUT_MS;
let timer: ReturnType<typeof setTimeout> | undefined;
- const result = await Promise.race([
- Promise.resolve(
- ssoController.validateSession({
- userId: authUser.userId,
- idpOrgId: authUser.sso.idpOrgId,
- connectionId: authUser.sso.connectionId,
- })
- ),
- new Promise<typeof REVALIDATION_TIMEOUT>((resolve) => {
- timer = setTimeout(() => resolve(REVALIDATION_TIMEOUT), timeoutMs);
- }),
- ]);
- if (timer) clearTimeout(timer);
+ let result: Awaited<ReturnType<typeof Promise.race>>;
+ try {
+ result = await Promise.race([
+ Promise.resolve(
+ ssoController.validateSession({
+ userId: authUser.userId,
+ idpOrgId: authUser.sso.idpOrgId,
+ connectionId: authUser.sso.connectionId,
+ })
+ ),
+ new Promise<typeof REVALIDATION_TIMEOUT>((resolve) => {
+ timer = setTimeout(() => resolve(REVALIDATION_TIMEOUT), timeoutMs);
+ }),
+ ]);
+ } catch (error) {
+ logger.warn("SSO revalidation threw; failing open (session kept alive)", {
+ userId: authUser.userId,
+ error,
+ });
+ return;
+ } finally {
+ if (timer) clearTimeout(timer);
+ }
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.
No description provided.