feat(webapp,rbac): REQUIRE_PLUGINS=1 fail-fast for required plugin loads [TRI-9852]#3734
feat(webapp,rbac): REQUIRE_PLUGINS=1 fail-fast for required plugin loads [TRI-9852]#3734matt-aitken wants to merge 4 commits into
Conversation
Adds a deployment-mode opt-in that hardens the plugin loader against silent degradation. Today, when the RBAC plugin module fails to load — whether because it's not installed or because a transitive dep is broken — the loader catches the error and returns the default fallback implementation. Correct for self-hosters; dangerous in deployments where the fallback's permissive auth is not an acceptable degraded state. - internal-packages/rbac/src/index.ts: in LazyController.load()'s catch block, after logging, throw an Error when `process.env.REQUIRE_PLUGINS === "1"`. The throw is captured into `this._init` (a rejected promise), so it surfaces on the first method call on the lazy controller. The forceFallback option (used by tests) still wins — it short-circuits before this code path, so tests aren't broken if a test runner happens to inherit REQUIRE_PLUGINS from its parent env. - apps/webapp/app/routes/healthcheck.tsx: call `rbac.isUsingPlugin()` after the DB ping. For self-hosters (and any deployment with the plugin loading cleanly) this is a noop. With REQUIRE_PLUGINS=1 and a failed plugin load, the throw surfaces here — healthcheck returns 500 and the rollout's readiness probe fails, so the new revision is rolled back before traffic shifts. REQUIRE_PLUGINS is intentionally plural and generic — future plugin contracts (audit logs, SSO) can read the same flag without renaming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four tests in require-plugins.test.ts driving the loader's branching: - REQUIRE_PLUGINS unset + plugin missing → falls back (resolves false) - REQUIRE_PLUGINS=1 + plugin missing → rejects on first method call - forceFallback: true beats REQUIRE_PLUGINS=1 (test escape hatch) - Any non-"1" value of REQUIRE_PLUGINS is treated as unset The plugin module isn't installed in this OSS repo, so the dynamic import naturally fails with ERR_MODULE_NOT_FOUND — no module mocking needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📜 Recent 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). (22)
🧰 Additional context used📓 Path-based instructions (9)**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{packages/core,apps/webapp}/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (9)📚 Learning: 2026-03-22T13:26:12.060ZApplied to files:
📚 Learning: 2026-03-22T19:24:14.403ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-18T08:21:27.694ZApplied to files:
📚 Learning: 2026-05-01T15:45:05.096ZApplied to files:
📚 Learning: 2026-05-09T08:07:24.612ZApplied to files:
📚 Learning: 2026-05-07T12:25:18.271ZApplied to files:
📚 Learning: 2026-05-12T21:04:05.815ZApplied to files:
📚 Learning: 2026-05-18T14:40:02.173ZApplied to files:
🔇 Additional comments (3)
WalkthroughAdds an opt-in fail-fast mode for the RBAC lazy plugin loader controlled by REQUIRE_PLUGINS=1 (import failures throw instead of falling back). The healthcheck route now awaits rbac.isUsingPlugin() so plugin-load failures surface during readiness probes. Adds unit tests and E2E tests, testcontainer wiring to spawn servers with REQUIRE_PLUGINS=1, and documentation describing the change. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.server-changes/require-plugins-fail-fast.md:
- Line 6: Update the documentation sentence about the REQUIRE_PLUGINS env var to
state that the variable must be set to the exact string "1" (not other truthy
values like "true" or "yes") to enable fail-fast plugin loading; reference the
REQUIRE_PLUGINS flag and the RBAC plugin loader behavior so readers know this
exact value triggers the throw and that the webapp healthcheck resolves the lazy
plugin controller to surface the failure during readiness probes.
In `@apps/webapp/app/routes/healthcheck.tsx`:
- Line 4: The Express bypass for healthcheck must perform the same readiness
checks as the Remix loader; update the app.get("/healthcheck", ...) handler in
server.ts to either call the same readiness logic used by the Remix route
(invoke rbac.isUsingPlugin() and any DB connection checks) or forward the
request to the Remix createRequestHandler so the loader in
app/routes/healthcheck.tsx runs; specifically ensure the handler invokes
rbac.isUsingPlugin() (and the same DB ping used by the Remix loader) and returns
200 only when those checks pass, otherwise return an error status.
🪄 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: CHILL
Plan: Pro
Run ID: e844eb70-0e93-4d74-9a3b-9464eb8d7ae4
📒 Files selected for processing (4)
.server-changes/require-plugins-fail-fast.mdapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.tsinternal-packages/rbac/src/require-plugins.test.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). (21)
- GitHub Check: internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: typecheck / typecheck
- GitHub Check: audit
- GitHub Check: 🛡️ E2E Auth Tests (full)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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 instead
Files:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
**/*.{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 otherwise, code splitting is needed for performance, or the module must be loaded conditionally at runtime.
Import from@trigger.dev/coreusing subpaths only - never import from the root.
When writing Trigger.dev tasks, always import from@trigger.dev/sdk. Never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Add agentcrumbs markers (//@Crumbsor `#region `@crumbs) as you write code, not just when debugging. They stay on the branch throughout development and are stripped byagentcrumbs stripbefore merge.
Files:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.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/rbac/src/require-plugins.test.ts
**/*.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:
internal-packages/rbac/src/require-plugins.test.tsinternal-packages/rbac/src/index.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Code formatting must be enforced using Prettier before committing
Files:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Test files should live beside the files under test and use descriptive describe and it blocks
Unit tests should use vitest framework
Tests should avoid mocks or stubs and use helpers from@internal/testcontainerswhen Redis or Postgres are needed
**/*.test.{ts,tsx,js,jsx}: 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/rbac/src/require-plugins.test.ts
{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/routes/healthcheck.tsx
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/routes/healthcheck.tsx
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/healthcheck.tsx
🧠 Learnings (12)
📚 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/require-plugins-fail-fast.md
📚 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:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
📚 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:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
📚 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:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
📚 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:
internal-packages/rbac/src/require-plugins.test.tsapps/webapp/app/routes/healthcheck.tsxinternal-packages/rbac/src/index.ts
📚 Learning: 2026-05-01T15:45:05.096Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3499
File: internal-packages/rbac/src/fallback.ts:34-107
Timestamp: 2026-05-01T15:45:05.096Z
Learning: When reviewing triggerdotdev/trigger.dev RBAC auth code, do not treat missing Personal Access Token (PAT) handling inside `authenticateBearer` as a bug. `authenticateBearer` is intentionally scoped to runtime environment API keys and Public JWTs only; PAT auth is handled via the separate PAT route builder (e.g., `createLoaderPATApiRoute`) which calls `authenticateApiRequestWithPersonalAccessToken` directly. Ensure that reviewers compare auth behavior against these distinct architectural paths (OSS fallback and cloud plugin) before flagging an issue.
Applied to files:
internal-packages/rbac/src/require-plugins.test.tsinternal-packages/rbac/src/index.ts
📚 Learning: 2026-05-09T08:07:24.612Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3499
File: internal-packages/rbac/src/fallback.ts:271-277
Timestamp: 2026-05-09T08:07:24.612Z
Learning: When reviewing RBAC/auth code that looks up or validates `PersonalAccessToken` (PAT), do not flag missing `expiresAt`/expiration checks: the PAT model has no `expiresAt` column and is treated as perpetual until manually revoked via `revokedAt`. Only require/enforce expiration logic when the code is dealing with `OrganizationAccessToken`, which does have an `expiresAt` field (and should be checked accordingly).
Applied to files:
internal-packages/rbac/src/require-plugins.test.tsinternal-packages/rbac/src/index.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/rbac/src/require-plugins.test.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/healthcheck.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/healthcheck.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/healthcheck.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/routes/healthcheck.tsx
🪛 LanguageTool
.server-changes/require-plugins-fail-fast.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...sing, broken transitive dep, etc.). The webapp's /healthcheck route now resolves the l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (4)
.server-changes/require-plugins-fail-fast.md (2)
1-4: LGTM!
8-8: LGTM!internal-packages/rbac/src/index.ts (1)
108-120: LGTM!internal-packages/rbac/src/require-plugins.test.ts (1)
1-44: LGTM!
| type: feature | ||
| --- | ||
|
|
||
| Add `REQUIRE_PLUGINS=1` env var. When set, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back. |
There was a problem hiding this comment.
Clarify that the value must be exactly "1".
The phrase "When set" could be misinterpreted as accepting any truthy value (e.g., true, yes). Per the test suite and implementation, only the exact string "1" triggers the fail-fast behavior. Consider rephrasing for clarity.
📝 Suggested clarification
-Add `REQUIRE_PLUGINS=1` env var. When set, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back.
+Add `REQUIRE_PLUGINS` env var. When set to exactly `"1"`, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back.📝 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.
| Add `REQUIRE_PLUGINS=1` env var. When set, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back. | |
| Add `REQUIRE_PLUGINS` env var. When set to exactly `"1"`, the RBAC plugin loader throws instead of silently falling back to the default implementation if the plugin module fails to load (missing, broken transitive dep, etc.). The webapp's `/healthcheck` route now resolves the lazy plugin controller so the throw surfaces during readiness probes — a deploy where the plugin didn't load fails the probe and is rolled back. |
🧰 Tools
🪛 LanguageTool
[grammar] ~6-~6: Ensure spelling is correct
Context: ...sing, broken transitive dep, etc.). The webapp's /healthcheck route now resolves the l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.server-changes/require-plugins-fail-fast.md at line 6, Update the
documentation sentence about the REQUIRE_PLUGINS env var to state that the
variable must be set to the exact string "1" (not other truthy values like
"true" or "yes") to enable fail-fast plugin loading; reference the
REQUIRE_PLUGINS flag and the RBAC plugin loader behavior so readers know this
exact value triggers the throw and that the webapp healthcheck resolves the lazy
plugin controller to surface the failure during readiness probes.
| import { prisma } from "~/db.server"; | ||
| import type { LoaderFunction } from "@remix-run/node"; | ||
| import { env } from "~/env.server"; | ||
| import { rbac } from "~/services/rbac.server"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SERVER_FILE="$(fd -i '^server\.ts$' apps/webapp | head -n 1)"
HEALTHCHECK_ROUTE="$(fd -i '^healthcheck\.tsx$' apps/webapp/app/routes | head -n 1)"
echo "Server file: ${SERVER_FILE}"
echo "Healthcheck route: ${HEALTHCHECK_ROUTE}"
echo
echo "=== /healthcheck and request-handler wiring in server.ts ==="
rg -n -C8 'app\.get\("/healthcheck"|createRequestHandler|app\.(all|use)\(' "${SERVER_FILE}"
echo
echo "=== healthcheck route loader ==="
sed -n '1,140p' "${HEALTHCHECK_ROUTE}"Repository: triggerdotdev/trigger.dev
Length of output: 5540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SERVER_FILE="apps/webapp/server.ts"
echo "=== All /healthcheck references in server.ts ==="
rg -n '"/healthcheck"|healthcheck' "$SERVER_FILE"
echo
echo "=== Re-check the exact conditional branches around /healthcheck ==="
rg -n -C5 'DASHBOARD_AND_API_DISABLED|app\.get\("/healthcheck"' "$SERVER_FILE"Repository: triggerdotdev/trigger.dev
Length of output: 1280
Ensure /healthcheck actually runs the Remix loader (rbac readiness) in production
apps/webapp/server.ts only registers an Express bypass for /healthcheck (app.get("/healthcheck", ...) returning plain 200 OK) when process.env.DASHBOARD_AND_API_DISABLED === "true", which skips apps/webapp/app/routes/healthcheck.tsx entirely (so await rbac.isUsingPlugin() won’t affect readiness).
When DASHBOARD_AND_API_DISABLED !== "true", /healthcheck is handled by the Remix createRequestHandler via app.all("*", ...), and the /realtime-only middleware explicitly allows req.url === "/healthcheck", so the Remix loader should run there.
If readiness probes can hit /healthcheck with DASHBOARD_AND_API_DISABLED=true, the Express handler needs to perform the same DB + rbac.isUsingPlugin() checks (or delegate to the Remix loader).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/webapp/app/routes/healthcheck.tsx` at line 4, The Express bypass for
healthcheck must perform the same readiness checks as the Remix loader; update
the app.get("/healthcheck", ...) handler in server.ts to either call the same
readiness logic used by the Remix route (invoke rbac.isUsingPlugin() and any DB
connection checks) or forward the request to the Remix createRequestHandler so
the loader in app/routes/healthcheck.tsx runs; specifically ensure the handler
invokes rbac.isUsingPlugin() (and the same DB ping used by the Remix loader) and
returns 200 only when those checks pass, otherwise return an error status.
Closes the loop on the unit-test loader coverage by spawning a real webapp + DB + Redis and verifying via HTTP: - REQUIRE_PLUGINS=1 + plugin missing → GET /healthcheck → 500 (so ECS/k8s readiness probes fail and the rollout rolls back). - REQUIRE_PLUGINS unset + plugin missing → GET /healthcheck → 200 (baseline — self-hoster behaviour unchanged). - internal-packages/testcontainers/src/webapp.ts: adds `requirePlugins?: boolean` to StartWebappOptions. Implies `forceRbacFallback: false` (you can't observe the throw if the loader short-circuits to the fallback). - apps/webapp/test/healthcheck-require-plugins.e2e.test.ts: two describes, each with its own webapp instance (~30s spawn each) since they need different env. Auto-picked up by vitest.e2e.config.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ly work Three small fixes uncovered by running the e2e test locally: - internal-packages/rbac/src/index.ts: attach a no-op .catch() to LazyController._init in the constructor. load() runs eagerly but its result is only awaited on the first method call. When load() rejects (REQUIRE_PLUGINS=1 + plugin missing), Node flags the unawaited rejection as unhandledRejection — newer Node versions kill the process before any consumer can await _init and convert the throw into a 500. The .catch() marks the rejection as handled at the global level; the actual error still re-throws when c() awaits _init. - internal-packages/testcontainers/src/webapp.ts: when requirePlugins is set, explicitly override RBAC_FORCE_FALLBACK="0" so a local apps/webapp/.env that sets it to "1" doesn't short-circuit the loader past the REQUIRE_PLUGINS check. Without this, the test passes in CI (no .env file) but fails locally with no obvious error. - internal-packages/testcontainers/src/webapp.ts: waitForHealthcheck now takes an `acceptAnyResponse` flag. When requirePlugins is set, /healthcheck is *expected* to return 500 (the whole point of the test), so the bootstrap waiter must accept any HTTP response rather than only 200 — otherwise startTestServer times out before the test can assert. apps/webapp/test/healthcheck-require-plugins.e2e.test.ts: tidied up the locally-linked-skip placeholder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🚩 Parent process REQUIRE_PLUGINS env var leaks into spawned webapp via ...process.env
The ...process.env spread at line 93 forwards ALL env vars from the test runner to the spawned webapp. If the test runner's environment happens to have REQUIRE_PLUGINS=1 set (e.g., from a CI pipeline variable), the spawned webapp will inherit it even when requirePlugins: false. There's no explicit REQUIRE_PLUGINS: undefined override for the non-requirePlugins case. This is unlikely to cause issues in practice (CI environments typically don't set this), and matches the existing pattern for other env vars, but worth being aware of.
(Refers to line 93)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // during readiness probes. With REQUIRE_PLUGINS=1, a failed plugin | ||
| // load throws here and the rollout's readiness probe fails. Without | ||
| // REQUIRE_PLUGINS, the fallback resolves cleanly and this is a noop. | ||
| await rbac.isUsingPlugin(); |
There was a problem hiding this comment.
🟡 HEALTHCHECK_DATABASE_DISABLED=1 silently bypasses the REQUIRE_PLUGINS check
The new rbac.isUsingPlugin() call is placed after the HEALTHCHECK_DATABASE_DISABLED early return at apps/webapp/app/routes/healthcheck.tsx:8-9. When HEALTHCHECK_DATABASE_DISABLED=1 is set, the function returns "OK" before ever reaching the RBAC check, so REQUIRE_PLUGINS=1 has no effect and the readiness probe passes even if the plugin is missing. The isUsingPlugin() call doesn't depend on DB connectivity (the fallback returns false synchronously per internal-packages/rbac/src/fallback.ts:63-64), so it should run even when the DB portion is disabled.
Prompt for agents
The rbac.isUsingPlugin() call at healthcheck.tsx:18 is placed after the HEALTHCHECK_DATABASE_DISABLED early return (lines 8-9), meaning it never runs when that env var is set. Since the rbac check does not need database connectivity (fallback.ts:63-64 returns false without touching prisma), it should be moved before the HEALTHCHECK_DATABASE_DISABLED guard so the REQUIRE_PLUGINS=1 protection works regardless of the database healthcheck setting. One approach is to move the await rbac.isUsingPlugin() call to just above the HEALTHCHECK_DATABASE_DISABLED check, or immediately below the try{ line.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
internal-packages/rbac/src/index.ts— inLazyController.load()'s catch block, throw an Error whenprocess.env.REQUIRE_PLUGINS === "1"instead of silently falling back. The throw is captured into the lazy controller's init promise, so it surfaces on the first method call.apps/webapp/app/routes/healthcheck.tsx—await rbac.isUsingPlugin()after the DB ping. WithREQUIRE_PLUGINS=1and a failed plugin load, the throw surfaces here and the healthcheck returns 500 → readiness probe fails → rollout is rolled back. Noop for self-hosters..server-changes/require-plugins-fail-fast.md— server-changes entry.internal-packages/rbac/src/require-plugins.test.ts— 4 unit tests covering loader branching: unset → fallback,=1→ throw,forceFallback: truewins, only exactly"1"enforces.internal-packages/testcontainers/src/webapp.ts— addsrequirePlugins?: booleantoStartWebappOptions. ImpliesforceRbacFallback: false.apps/webapp/test/healthcheck-require-plugins.e2e.test.ts— e2e closes the loop: spawns a real webapp, hits/healthcheckvia HTTP, asserts 500 withREQUIRE_PLUGINS=1and 200 without.Motivation
Today the RBAC plugin loader catches any plugin-load failure (missing module, broken transitive dep, init throw) and silently returns the default fallback implementation. This is the correct behaviour for self-hosters who don't ship the plugin — but it's dangerous in deployments where the plugin is expected to load: an accidentally-missing or broken plugin would silently disable enforcement.
REQUIRE_PLUGINS=1makes the loader fail loudly in those deployments. The variable name is intentionally plural and generic — future plugin contracts (audit logs, SSO) can read the same flag without renaming.Closes TRI-9852.
Test plan
pnpm run test --filter @trigger.dev/rbac— 38/38 tests pass, including the 4 new loader testspnpm run typecheck --filter webapp— passespnpm run typecheck --filter @trigger.dev/rbac --filter @internal/testcontainers— passeshealthcheck-require-plugins.e2e.test.ts) — CI runs it viae2e-webapp.yml. Couldn't run locally (no Docker daemon up); CI has Docker provisioned.🤖 Generated with Claude Code