feat(webapp): let the deployment S2 client endpoints be overridden - #4867
feat(webapp): let the deployment S2 client endpoints be overridden#4867d-cs wants to merge 1 commit into
Conversation
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds optional S2 deployment endpoint configuration with validation for HTTPS and loopback HTTP URLs. A shared client factory maps the endpoint to both account and basin hosts and preserves hosted defaults when unset. Deployment presenters and services now use the shared factory. Tests cover endpoint validation, client options, local routing, and hosted configuration. Merge Risk: 🔵 Low · up to The PR enables deployment logs to use a configured local or alternate S2 service while preserving hosted defaults when unset. A misconfigured endpoint could send the S2 credential and log traffic to the wrong destination, though validation limits this to HTTPS or loopback HTTP; the change is mergeable with explicit owner awareness and minor follow-up for test configuration coupling. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description gives a detailed summary, design rationale, scope, configuration requirements, testing result, and known limitations. It does not include the template's issue-closing line, checklist, or screenshots section, but the missing sections do not prevent understanding or reviewing the change. ✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
45f6f1f to
52b7ade
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
0f09b92 to
789232b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2511851 to
c7a6e87
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/webapp/test/s2Endpoint.test.ts (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the test next to its source file.
Move
apps/webapp/test/s2Endpoint.test.tstoapps/webapp/app/utils/s2Endpoint.test.ts. This keeps the test aligned with the repository test-location rule.As per coding guidelines, test files go next to source files.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: f2b5fc30-0d0b-4182-99fd-fa6255e49dfe
📒 Files selected for processing (3)
apps/webapp/app/env.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/test/s2Endpoint.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/env.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
🧰 Additional context used
📓 Path-based instructions (13)
Never use `request.signal` to detect client disconnects.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/utils/s2Endpoint.ts
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/s2Endpoint.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/s2Endpoint.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/utils/s2Endpoint.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/s2Endpoint.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2Endpoint.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.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 metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/s2Endpoint.test.tsapps/webapp/app/utils/s2Endpoint.ts
🔇 Additional comments (2)
apps/webapp/app/utils/s2Endpoint.ts (2)
13-17: 🔒 Security & PrivacyAdd a redirect regression test for S2 requests.
Node’s global
fetchstripsAuthorizationwhen a redirect changes origin, including a scheme change. Lock this behavior with a recording-target test before relying onisValidS2Endpointas the transport control.
17-17: 🔒 Security & PrivacySensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Confirm that loopback S2 requests bypass proxies.
isValidS2Endpointaccepts HTTP loopback URLs, and the SDK sendsS2_ACCESS_TOKENthroughglobalThis.fetch. Add a bounded Node 24.18.0 integration test with a recording proxy. Assert that the proxy receives noAuthorizationheader.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The S2 SDK honours only endpoints passed to its constructor. S2Environment.parse(), which reads the endpoint variables, is an opt-in helper the webapp never called, so deployment event logs always went to hosted S2 and could not be pointed at the local s2 container in docker compose. Realtime streams already had this knob. S2_DEPLOYMENT_ENDPOINT is a single value covering both the account and basin hosts. Two separate variables would let a half-set config send the access token to the hosted service while the operator believed the client was entirely local. With the variable unset the options object carries no endpoints key at all, so the call into the SDK is the one production already makes. An endpoints key with undefined members resolves to the same hosted URLs, so no assertion on the built client would catch a regression there; the options builder is asserted directly instead, from a module that reads no environment. The value is trimmed and validated. https is accepted anywhere, http only to a host that is not reachable from the public internet: loopback, a single-label container or service name as the self-hosted stack uses, or a private address. The access token is sent to whatever is configured as a bearer token, so cleartext to a routable host would leak it, and zod url() is too weak for this alone, accepting both "htp:/localhost" and any public http host. Cached S2 read tokens are scoped by endpoint. They are issued by whichever service the endpoint names and Redis outlives a restart, so a key scoped only by project would serve a token from the previous service for the rest of its hour. Hosted keeps its existing keys, so nothing is invalidated where the endpoint cannot change.
c7a6e87 to
6c16d8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 375134ec-e660-47de-9782-bc51fc410a3a
📒 Files selected for processing (8)
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/test/s2Endpoint.test.tsapps/webapp/vitest.config.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/v3/s2CacheScope.ts
Never use `request.signal` to detect client disconnects.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.ts
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/s2CacheScope.test.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/test/s2Endpoint.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/s2CacheScope.test.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/test/s2Endpoint.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/s2CacheScope.test.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/test/s2Endpoint.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` Use subpath exports from `@trigger.dev/core` package instead of importing from the root `@trigger.dev/core` path
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2CacheScope.test.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/test/s2Endpoint.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.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 metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/vitest.config.tsapps/webapp/test/s2CacheScope.test.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2ClientConfig.test.tsapps/webapp/app/v3/services/deployment.server.tsapps/webapp/app/utils/s2Endpoint.tsapps/webapp/app/v3/s2CacheScope.tsapps/webapp/test/s2Endpoint.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev
Timestamp: 2026-09-02T09:35:32.989Z
Learning: In `apps/webapp`, Vitest collects colocated tests under `app/` through explicit paths in the webapp Vitest configuration. When adding a colocated test such as `apps/webapp/app/v3/s2ClientConfig.test.ts`, add its path to that configuration alongside existing entries such as `apps/webapp/app/v3/runStore.server.test.ts`. Utility tests such as `apps/webapp/test/s2Endpoint.test.ts` and `apps/webapp/test/s2CacheScope.test.ts` remain in `apps/webapp/test` to match their existing non-colocated helper-test pattern.
📚 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/presenters/v3/DeploymentPresenter.server.ts
🔇 Additional comments (6)
apps/webapp/vitest.config.ts (1)
16-16: LGTM!apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (2)
16-21: LGTM!Also applies to: 290-290
34-34: 🩺 Stability & AvailabilityNo change needed for disabled S2 initialization.
createDeploymentS2Client()returnsundefinedbefore callingbuildDeploymentS2Client(), which is the only path that constructsnew S2(...).apps/webapp/app/v3/services/deployment.server.ts (1)
26-29: LGTM!Also applies to: 527-527
apps/webapp/app/v3/s2CacheScope.ts (1)
1-6: LGTM!apps/webapp/test/s2CacheScope.test.ts (1)
1-20: LGTM!
| @@ -0,0 +1,34 @@ | |||
| const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "::1", "[::1]"]); | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required crumbs instrumentation to the new code.
apps/webapp/app/utils/s2Endpoint.ts#L1-L1: add//@Crumbsmarkers or wrap the new validation block in a `// `#region` `@crumbsblock.apps/webapp/test/s2Endpoint.test.ts#L1-L1: add//@Crumbsmarkers or wrap the new test block in a `// `#region` `@crumbsblock.apps/webapp/app/v3/s2ClientConfig.test.ts#L1-L1: add//@Crumbsmarkers or wrap the new test block in a `// `#region` `@crumbsblock.
As per coding guidelines, “Add crumbs as you write code” and mark lines with // @Crumbs or `// `#region` `@crumbs.
📍 Affects 3 files
apps/webapp/app/utils/s2Endpoint.ts#L1-L1(this comment)apps/webapp/test/s2Endpoint.test.ts#L1-L1apps/webapp/app/v3/s2ClientConfig.test.ts#L1-L1
Source: Coding guidelines
| // A single-label hostname (no dot) is a container or service name on a private network, which is | ||
| // how the self-hosted stack reaches S2, e.g. `http://s2/v1`. | ||
| function isPrivateHost(hostname: string): boolean { | ||
| return LOOPBACK_HOSTS.has(hostname) || !hostname.includes(".") || isPrivateIpv4(hostname); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not classify every single-label hostname as private.
Line 16 accepts http://s2 without checking its resolved address. A DNS server or search suffix can resolve that name to a public host. This bypasses the HTTP restriction and can expose the bearer token in cleartext.
Require HTTPS for hostname-based endpoints, or enforce private resolved addresses in the transport for every connection. Do not rely on one-time DNS validation because DNS rebinding can change the destination.
1 participant
Summary
Deployment event logs always talked to hosted S2, so running the local
s2container fromdocker-composecould not serve them: the deploy start path fails with a server error. A single optional env var now points that client somewhere else.Unset means the SDK's hosted defaults, so nothing changes for existing deployments. The full local setup also needs
S2_ENABLED=1,S2_ACCESS_TOKEN, andS2_DEPLOYMENT_LOGS_BASIN_NAME=trigger-localto match the basindocker/config/s2-spec.jsoncreates.Design
The S2 SDK honours only the endpoints passed to its constructor.
S2Environment.parse(), which reads endpoint variables from the environment, is an opt-in helper we never called, so the client fell back toa.s2.devand{basin}.b.s2.devregardless of configuration. Realtime streams already had an equivalent knob.One value covers both the account and basin hosts on purpose. The SDK takes them separately, and setting only one leaves the other on the hosted default: the access token would go to hosted S2 over the public internet while the operator believed the client was entirely local, with nothing logged. A single value makes that state unrepresentable.
With the variable unset, the options object carries no
endpointskey at all, so the call into the SDK is exactly the one production already makes. That distinction is not observable on the built client, because anendpointskey whose members are undefined resolves to the same hosted URLs, so the options builder is exported and asserted directly. Putting the key back on the unset path fails the test.Scope
This covers writing deployment event logs. Reading them back is still hosted-only: the dashboard hook and the CLI build-log tail construct their own clients, so log viewing against a local S2 does not work yet. Worth doing as a follow-up, since the server already mints a local token those readers cannot use.
Verified against the local container: a deploy completes and the event stream is created in the local basin.