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
|
WalkthroughS2 configuration now supports an optional deployment endpoint override. A shared Merge Risk: 🟡 Moderate · up to The endpoint override changes where deployment credentials and logs are sent, but the current implementation can leave dashboard reads pointed at hosted S2, reuse cached tokens across endpoint changes, and fail startup for blank endpoint values. These create concrete credential-boundary and rollout risks, so merge should wait for fixes or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on-topic, and explains the change, design, scope, configuration, and validation. It does not use the repository template headings for the issue link, checklist, testing, changelog, or screenshots, and it describes one S2_DEPLOYMENT_ENDPOINT while the stated objectives name S2_ACCOUNT_ENDPOINT and S2_BASIN_ENDPOINT.
✨ 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 |
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. Unset means the SDK hosted defaults, so nothing changes for existing deployments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/webapp/app/v3/s2Client.server.ts (1)
6-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the required
@crumbsmarkers to the changed blocks.The
**/*guideline requires//@Crumbsor `// `#region` `@crumbswhile code is developed. Add markers to each listed block.agentcrumbs stripremoves them before merge.
apps/webapp/app/v3/s2Client.server.ts#L6-L17: mark the shared factory implementation.apps/webapp/app/env.server.ts#L82-L87: mark the new endpoint schema fields.apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts#L16-L16: mark the changed import.apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts#L33-L33: mark the factory initialization.apps/webapp/app/v3/services/deployment.server.ts#L26-L27: mark the changed imports.apps/webapp/app/v3/services/deployment.server.ts#L39-L39: mark the factory initialization.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 964eef39-a257-4fb5-9d0b-33fb97dfe0e3
📒 Files selected for processing (4)
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
🧰 Additional context used
📓 Path-based instructions (10)
New code must target Run Engine V2 through the singleton in `app/v3/runEngine.server.ts`; do not reintroduce V1 execution paths. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.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/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.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
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/app/v3/services/deployment.server.ts
🔇 Additional comments (1)
apps/webapp/app/v3/s2Client.server.ts (1)
16-16: 🎯 Functional Correctness
exactOptionalPropertyTypesis not enabled; no type error occurs.The tsconfig.json for apps/webapp does not set
exactOptionalPropertyTypes. With that option disabled (the default),undefinedis assignable to optional properties. The object{ account: string | undefined, basin: string | undefined }is valid for the parameter type{ account?: string, basin?: string }. The code requires no changes.
| accessToken: env.S2_ACCESS_TOKEN, | ||
| ...(account !== undefined || basin !== undefined ? { endpoints: { account, basin } } : {}), |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
# First, read the S2 client file to understand the complete context
cat -n apps/webapp/app/v3/s2Client.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 916
🏁 Script executed:
# Search for where S2_ACCOUNT_ENDPOINT and S2_BASIN_ENDPOINT are set
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT|S2_ACCESS_TOKEN" --type ts --type tsx -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 195
🏁 Script executed:
# Check package.json for S2 SDK version
cat -n package.json | grep -A 5 -B 5 "s2\|S2" || find . -name package.json -exec grep -l "s2" {} \;Repository: triggerdotdev/trigger.dev
Length of output: 250
🏁 Script executed:
# Check how deployment endpoints are configured
rg "createDeploymentS2Client\|S2_ENABLED" --type ts --type tsx -B 3 -A 3Repository: triggerdotdev/trigger.dev
Length of output: 195
🌐 Web query:
S2 StreamStore SDK version 0.25.0 HTTP endpoint support bearer token
💡 Result:
The S2 StreamStore SDK supports Bearer token authentication for all HTTP endpoint requests [1][2]. When using the official S2 SDKs, authentication is handled automatically [1][2]. You initialize the client by providing an access token, which the SDK then manages and includes in the Authorization header for all subsequent requests [2][3][4]. For example, in the TypeScript SDK, this is done by passing the accessToken parameter to the S2 client constructor [3][4]. If you are interacting with S2 HTTP endpoints directly without using an SDK (e.g., via curl), you must manually include the token in the request header using the following format [1][2]: Authorization: Bearer YOUR_ACCESS_TOKEN Access tokens are required for all S2 API requests and are managed through the S2 account API [2]. While the SDK abstracts this management, it remains consistent with the standard Bearer token authentication required by the S2 protocol [1][2]. SDK version 0.25.0, released in March 2026, continues to follow these established authentication patterns [5].
Citations:
- 1: https://s2.dev/docs/api/protocol
- 2: https://s2-streamstore-s2.mintlify.app/api/authentication
- 3: https://s2-streamstore-s2.mintlify.app/sdks/typescript
- 4: https://s2.dev/docs/sdk/languages
- 5: https://github.com/s2-streamstore/s2/blob/main/sdk/CHANGELOG.md
🏁 Script executed:
# Check webapp package.json for S2 SDK version
cat -n apps/webapp/package.json | head -100 | grep -A 5 -B 5 s2Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Search for env.server to understand where endpoints come from
cat -n apps/webapp/app/env.server.ts | head -150Repository: triggerdotdev/trigger.dev
Length of output: 6949
🏁 Script executed:
# Find references to S2_ACCOUNT_ENDPOINT using simpler rg search
rg "S2_ACCOUNT_ENDPOINT" -A 2 -B 2 apps/webapp/Repository: triggerdotdev/trigger.dev
Length of output: 846
🏁 Script executed:
# Check if S2 SDK is in apps/webapp or shared
find apps/webapp apps/cli-v3 packages/core -name package.json -exec grep -l "`@s2-dev`" {} \;Repository: triggerdotdev/trigger.dev
Length of output: 262
🏁 Script executed:
# Check the exact S2 SDK version in package.json
cat apps/webapp/package.json | grep -A 2 -B 2 '"`@s2-dev`'Repository: triggerdotdev/trigger.dev
Length of output: 343
🏁 Script executed:
# Check if there are any validation checks for endpoint scheme (http/https)
rg "http" apps/webapp/app/v3/s2Client.server.ts -A 5 -B 5Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Search for how S2 is actually used - any runtime validation of endpoints
rg "endpoint" apps/webapp/app/v3/ -i --max-count 20Repository: triggerdotdev/trigger.dev
Length of output: 4510
🏁 Script executed:
# Check if there's documentation or comments about expected endpoint format
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT" -B 5 -A 5 apps/webapp/Repository: triggerdotdev/trigger.dev
Length of output: 1884
🏁 Script executed:
# Look at the S2 SDK docs or types to see if it validates endpoint schemes
find . -name "*.ts" -path "*/node_modules/@s2-dev*" 2>/dev/null | head -5Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Check if there's any documentation about token scope or deployment mode
rg "S2_ENABLED|S2_ACCESS_TOKEN" apps/webapp/app/env.server.ts -B 3 -A 3Repository: triggerdotdev/trigger.dev
Length of output: 1416
🏁 Script executed:
# Check docker-compose or deployment docs for how s2-lite is meant to be used
find . -name "docker-compose*" -o -name ".env*" -o -name "README*" | head -10Repository: triggerdotdev/trigger.dev
Length of output: 395
🏁 Script executed:
# Look for any comments about credential handling or endpoint validation elsewhere
rg "S2|endpoint" apps/webapp/app/env.server.ts -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 4494
🏁 Script executed:
# Check if S2 SDK is available and examine its type definitions
cat apps/webapp/package.json | grep -A 5 "`@s2-dev/streamstore`"Repository: triggerdotdev/trigger.dev
Length of output: 378
🏁 Script executed:
# Check if there's endpoint scheme validation anywhere in the codebase
rg "https?://" apps/webapp/app/v3/s2Client.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Check the docker-compose to see how s2-lite is configured
cat docker/docker-compose.yml | grep -A 10 "s2:"Repository: triggerdotdev/trigger.dev
Length of output: 479
🏁 Script executed:
# See if there are any examples or docs about endpoint configuration
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT" .env.example docker/.env.example 2>/dev/nullRepository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Check if env.server.ts validates endpoint URLs anywhere with scheme checks
rg "https|protocol|scheme" apps/webapp/app/env.server.ts -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 1796
🏁 Script executed:
# Check if there's any scheme validation in env.server.ts when parsing the endpoint URLs
rg "z.string()" apps/webapp/app/env.server.ts | grep -A 2 -B 2 "S2_ACCOUNT_ENDPOINT\|S2_BASIN_ENDPOINT"Repository: triggerdotdev/trigger.dev
Length of output: 482
🏁 Script executed:
# Check how REALTIME_STREAMS urls are defined to see if there's a pattern we should follow
rg "REALTIME_STREAMS_S2" apps/webapp/app/env.server.ts -B 1 -A 1 | head -20Repository: triggerdotdev/trigger.dev
Length of output: 1257
🏁 Script executed:
# Look for any URL validation utilities in the codebase
rg "url|endpoint|scheme" apps/webapp/app/env.server.ts | grep -i "refine\|parse\|validate"Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Check if there's any validation of S2_ACCOUNT_ENDPOINT/S2_BASIN_ENDPOINT in the SDK consumer code
rg "createDeploymentS2Client\|S2_ACCOUNT_ENDPOINT\|S2_BASIN_ENDPOINT" apps/webapp/app/v3/ -B 2 -A 5Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Check if there's any middleware or validation that happens before the S2 client is created
rg "createDeploymentS2Client" apps/webapp/app/ -B 5 -A 5 | head -40Repository: triggerdotdev/trigger.dev
Length of output: 3631
🏁 Script executed:
# Verify the complete env schema to see if there's any superRefine or post-processing validation
cat -n apps/webapp/app/env.server.ts | sed -n '67,95p'Repository: triggerdotdev/trigger.dev
Length of output: 1421
🏁 Script executed:
# Check if there's any attempt to use different credentials for local vs hosted S2
rg "s2-lite|local|localhost" apps/webapp/app/v3/ -i -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 13875
🏁 Script executed:
# Check how the S2 client is used - what operations happen
rg "s2\." apps/webapp/app/v3/services/deployment.server.ts -B 2 -A 2 | head -50Repository: triggerdotdev/trigger.dev
Length of output: 718
🏁 Script executed:
# Look for any documentation about S2 token scope or mode
cat -n apps/webapp/app/env.server.ts | sed -n '67,93p'Repository: triggerdotdev/trigger.dev
Length of output: 1329
🏁 Script executed:
# Check if there's any validation or transformation of endpoints anywhere else
rg "endpoint" apps/webapp/app/ -i | grep -i "s2\|validate\|scheme\|http"Repository: triggerdotdev/trigger.dev
Length of output: 3614
🏁 Script executed:
# Final verification: check if there's any token validation or scoping
rg "S2_ACCESS_TOKEN|accessToken" apps/webapp/app/v3/s2Client.server.ts -B 3 -A 3Repository: triggerdotdev/trigger.dev
Length of output: 363
🏁 Script executed:
# Check if there's any attempt to differentiate tokens for local vs prod
rg "accessToken" apps/webapp/app/v3/services/deployment.server.ts -B 2 -A 2 | head -20Repository: triggerdotdev/trigger.dev
Length of output: 507
🏁 Script executed:
# Verify the complete flow: env -> factory -> usage
cat -n apps/webapp/app/v3/s2Client.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 916
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal · Exploitability: Difficult
Validate S2 endpoint schemes to prevent sending credentials over unencrypted connections.
createDeploymentS2Client() always provides env.S2_ACCESS_TOKEN to the S2 SDK. When custom endpoints (S2_ACCOUNT_ENDPOINT, S2_BASIN_ENDPOINT) are set, the factory passes them directly to the S2 constructor without validating the URI scheme. The S2 SDK version 0.25.0 sends the bearer token in the Authorization header to all endpoints. If an endpoint uses http://, the token travels unencrypted and can be intercepted by a network observer.
Add scheme validation in env.server.ts to require HTTPS for credentialed endpoints, or implement a separate non-privileged token path for local s2-lite development (currently the docker-compose service runs on HTTP port 80 and reuses the production token).
Source: MCP tools
|
|
||
| return new S2({ | ||
| accessToken: env.S2_ACCESS_TOKEN, | ||
| ...(account !== undefined || basin !== undefined ? { endpoints: { account, basin } } : {}), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
# Find and read the s2Client.server.ts file to understand current implementation
find . -name "s2Client.server.ts" -type fRepository: triggerdotdev/trigger.dev
Length of output: 203
🏁 Script executed:
# Find the environment schema/validation files
find apps/webapp -name "*env*" -type f | head -20Repository: triggerdotdev/trigger.dev
Length of output: 1347
🏁 Script executed:
# Look for zod validation schemas related to S2 endpoints
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT" --type ts --type tsx -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 195
🏁 Script executed:
# Read the s2Client.server.ts file to see the actual implementation
cat -n apps/webapp/app/v3/s2Client.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 916
🏁 Script executed:
# Search for S2 endpoint configuration
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT" -A 2 -B 2Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Find the env.server.ts file in apps/webapp
cat -n apps/webapp/app/env.server.ts | head -150Repository: triggerdotdev/trigger.dev
Length of output: 6949
🏁 Script executed:
# Check package.json for `@s2-dev/streamstore` version
rg "`@s2-dev/streamstore`" apps/webapp/package.json -A 1 -B 1Repository: triggerdotdev/trigger.dev
Length of output: 275
🏁 Script executed:
# Look for more context about S2 SDK usage or issues
rg "endpoints.*account.*basin" apps/webapp -A 3 -B 3Repository: triggerdotdev/trigger.dev
Length of output: 1149
🏁 Script executed:
# Check if there are any tests or examples showing S2 endpoint behavior
find apps/webapp -name "*.test.ts" -o -name "*.test.tsx" | xargs rg -l "S2" 2>/dev/null | head -5Repository: triggerdotdev/trigger.dev
Length of output: 427
🏁 Script executed:
# Let me verify the schema more completely - check if there's preprocessing for S2 endpoints
cat -n apps/webapp/app/env.server.ts | sed -n '67,93p'Repository: triggerdotdev/trigger.dev
Length of output: 1329
🏁 Script executed:
# Check if there are any GitHub Actions or test configurations that set empty S2 endpoints
rg "S2_ACCOUNT_ENDPOINT|S2_BASIN_ENDPOINT" --type json --type yaml -A 1 -B 1Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Look at the sessionStream test helper to see how endpoints are used
cat -n apps/webapp/test/helpers/sessionStream.tsRepository: triggerdotdev/trigger.dev
Length of output: 11734
🌐 Web query:
@s2-dev/streamstore 0.25.0 endpoints validation empty string
💡 Result:
There is no specific, documented issue in the @s2-dev/streamstore 0.25.0 release regarding endpoints validation for empty strings [1][2]. The @s2-dev/streamstore library serves as the official TypeScript SDK for S2, a serverless stream store [3][2]. The 0.25.0 release primarily focused on infrastructure updates, specifically migrating S2 host endpoints (e.g., updating basin hostnames to {basin}.b.s2.dev) and improving session-stream reliability [1][4]. If you are encountering issues related to empty strings in endpoints, it is likely due to the underlying REST API requirements or standard validation logic rather than a known regression or bug in version 0.25.0 [1][5]. If you suspect a bug, it is recommended to check the official repository for existing issues or reports regarding parameter validation [3][6]. General notes on S2 endpoint and input validation: - The SDK relies on the S2 REST API, where endpoint parameters (like basin or stream names) are expected to be non-empty strings [5]. - Input validation errors in the SDK are typically surfaced through S2Error subclasses, such as InvalidRequestError [7][8]. - You can inspect error details by catching S2Error and examining the status, headers, or raw response body if the server returns a 400 Bad Request due to validation failure [7][8].
Citations:
- 1: GitHub pull request 4349 in triggerdotdev/trigger.dev (link omitted to avoid creating a cross-reference)
- 2: https://s2-streamstore.github.io/s2-sdk-typescript/
- 3: https://github.com/s2-streamstore/s2-sdk-typescript
- 4: https://s2.dev/docs/updates
- 5: https://s2.dev/
- 6: GitHub issue 280 in s2-streamstore/s2-sdk-typescript (link omitted to avoid creating a cross-reference)
- 7: https://github.com/s2-streamstore/s2-sdk-typescript-old
- 8: https://github.com/s2-streamstore/s2-sdk-typescript-old/blob/main/README.md
🏁 Script executed:
# Verify what z.string().optional() actually accepts by looking at test patterns in the codebase
rg "z\.string\(\)\.optional\(\)" apps/webapp/app/env.server.ts -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 20064
🏁 Script executed:
# Check if there's any preprocessing or transformation applied to S2 endpoint fields
sed -n '76,88p' apps/webapp/app/env.server.tsRepository: triggerdotdev/trigger.dev
Length of output: 814
🏁 Script executed:
# Look for any test files that might exercise empty endpoint values
find apps/webapp/test -name "*.test.ts" -exec grep -l "S2.*endpoint\|endpoint.*S2" {} \;Repository: triggerdotdev/trigger.dev
Length of output: 163
🏁 Script executed:
# Verify the S2 SDK behavior by checking the endpoints.ts file from the referenced URL
python3 - <<'PY'
# Simulate what z.string().optional() accepts
from typing import Optional, Union
# z.string().optional() in zod parses as:
# - Accepts any non-null string (including "")
# - Accepts undefined/null which becomes undefined
# - Does NOT have additional validation for non-empty strings
test_values = ["", "http://example.com", undefined_placeholder := None]
for val in ["", "http://example.com", None]:
# z.string().optional() behavior:
if val is None:
parsed = None # undefined
elif isinstance(val, str):
parsed = val # any string including ""
print(f"Input: {repr(val)} -> Parsed: {repr(parsed)}")
# The !== undefined check in s2Client
def passes_undefined_check(val):
return val is not None # account !== undefined in TypeScript
print("\n!== undefined check:")
print(f"Empty string: {passes_undefined_check('')}")
print(f"Valid endpoint: {passes_undefined_check('http://example.com')}")
print(f"Undefined: {passes_undefined_check(None)}")
PYRepository: triggerdotdev/trigger.dev
Length of output: 354
🏁 Script executed:
# Check if there's existing error handling or documentation about SDK failures
rg "S2(" apps/webapp/app/v3/s2Client.server.ts -A 10 -B 2Repository: triggerdotdev/trigger.dev
Length of output: 226
🏁 Script executed:
# Verify if empty endpoint strings would actually reach the S2 constructor
python3 - <<'PY'
# Simulate the code path with empty endpoint
account = "" # from env var (passes z.string().optional())
basin = "" # from env var (passes z.string().optional())
# Check the condition at line 16
if account is not None or basin is not None:
# This evaluates to True with empty strings
endpoints = {"account": account, "basin": basin}
print(f"Empty string endpoints passed to S2: {endpoints}")
else:
endpoints = None
print("No endpoints passed")
# Verify that empty strings are truthy
print(f"\n'' !== undefined: {account is not None}")
print(f"Actual value passed: {repr(account)}")
PYRepository: triggerdotdev/trigger.dev
Length of output: 277
Empty S2 endpoint environment variables bypass the !== undefined check and can cause SDK initialization failures.
z.string().optional() without preprocessing accepts empty strings and treats them as configured values. The check at line 16 (account !== undefined) passes for empty strings, causing the S2 constructor to receive { endpoints: { account: "", basin: "" } } instead of no endpoints object. The S2 SDK v0.25.0 expects valid endpoint URLs and rejects empty strings during initialization.
Normalize blank endpoint values before constructing S2. The repository already uses this pattern for other optional string environment variables (see OptionalIntEnv, OptionalBoolEnv in env.server.ts).
Proposed fix
- const account = env.S2_ACCOUNT_ENDPOINT;
- const basin = env.S2_BASIN_ENDPOINT;
+ const account = env.S2_ACCOUNT_ENDPOINT?.trim() || undefined;
+ const basin = env.S2_BASIN_ENDPOINT?.trim() || undefined;📝 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.
| ...(account !== undefined || basin !== undefined ? { endpoints: { account, basin } } : {}), | |
| const account = env.S2_ACCOUNT_ENDPOINT?.trim() || undefined; | |
| const basin = env.S2_BASIN_ENDPOINT?.trim() || undefined; | |
| ...(account !== undefined || basin !== undefined ? { endpoints: { account, basin } } : {}), |
Source: MCP tools
45f6f1f to
52b7ade
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 87cc9eef-986d-4a7a-9b1b-afc8aee67426
📒 Files selected for processing (3)
apps/webapp/app/env.server.tsapps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (28)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 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. V1 branches may only reject or finalize gracefully with a clean 4xx.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/s2Client.server.ts
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/app/env.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/s2Client.server.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/s2Client.server.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/v3/s2Client.server.tsapps/webapp/app/env.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.ts
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.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/s2Client.server.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/s2Client.server.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.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
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/app/v3/s2Client.server.tsapps/webapp/test/s2Client.server.test.tsapps/webapp/app/env.server.ts
🧠 Learnings (2)
📚 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/v3/s2Client.server.ts
📚 Learning: 2026-05-28T20:02:10.647Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 3772
File: apps/webapp/test/findOrCreateBackgroundWorker.test.ts:1-1
Timestamp: 2026-05-28T20:02:10.647Z
Learning: In the triggerdotdev/trigger.dev monorepo, for the `apps/webapp` package use the established convention of storing Vitest tests (unit, integration, and e2e) under `apps/webapp/test/` rather than colocating them next to source files. Do not flag files located in `apps/webapp/test/` as violating any rule that says to colocate tests with source.
Applied to files:
apps/webapp/test/s2Client.server.test.ts
| // Points deployment event logs at an S2 service other than the hosted one, e.g. the | ||
| // local s2-lite in docker compose. One value covers both the account and basin | ||
| // endpoints: splitting them lets a half-set config send the access token to the | ||
| // hosted service while the operator believes they are entirely local. | ||
| S2_DEPLOYMENT_ENDPOINT: z | ||
| .string() | ||
| .url() | ||
| .optional() | ||
| .or(z.literal("").transform(() => undefined)), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add crumbs for the endpoint-override path.
Add a crumb marker or #region @crumbs`` block while implementing this configuration path. Use an approved namespace, or ask before selecting one.
apps/webapp/app/env.server.ts#L82-L90: mark the endpoint parsing and normalization path.apps/webapp/app/v3/s2Client.server.ts#L12-L16: mark the endpoint client-construction path.
As per coding guidelines: “Add crumbs as you write code” and “Do not invent new namespaces.”
📍 Affects 2 files
apps/webapp/app/env.server.ts#L82-L90(this comment)apps/webapp/app/v3/s2Client.server.ts#L12-L16
Source: Coding guidelines
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, and the accompanying test pins both the hosted default and the override.
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.