Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
|
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:
WalkthroughReplaces the R2-specific object-store implementation with a new multi-protocol object store module ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
🧭 Helm Chart Prerelease PublishedVersion: Install: helm upgrade --install trigger \
oci://ghcr.io/triggerdotdev/charts/trigger \
--version "4.0.5-pr3275.dd0891e"
|
![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.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/webapp/app/v3/objectStore.server.ts`:
- Around line 24-35: parseStorageUri currently only matches lowercase protocols
so mixed/uppercase protocols (e.g. "S3://...") become mis-parsed later; update
parseStorageUri to normalize the protocol to a canonical case (lowercase) before
returning and add a zod schema to validate/normalize the incoming URI at the
boundary so callers cannot persist an uppercased protocol; ensure the same
normalization/validation is applied where you format URIs (referenced around the
other block lines and constants like OBJECT_STORE_DEFAULT_PROTOCOL and
getObjectStoreConfig) so stored URIs always use the normalized protocol.
- Around line 362-369: The debug log is leaking live presigned credentials by
logging signed.url and raw signed.headers; update the logger.debug call so it
never emits signed.url or full header values—either remove those fields entirely
or replace them with redacted placeholders (e.g., "<REDACTED_URL>" and masked
header keys), and instead log safe metadata such as expiry/TTL or a boolean
indicating presence of auth headers; specifically change the logger.debug
invocation that references signed.url and Object.fromEntries(signed.headers) to
omit or mask those values while keeping projectRef, envSlug, filename, and
protocol.
- Around line 212-215: The logger call inside the packet.data guard currently
serializes the full environment; instead sanitize and log only non-sensitive
identifiers: replace the environment object in the logger.error call with a
minimal payload like { projectExternalRef: environment?.project?.externalRef,
slug: environment?.slug, packetShape: { id: packet?.id, type: packet?.type,
hasData: Boolean(packet?.data) } } so credentials are not emitted; update the
logger.error invocation (the call that currently passes { packet, environment })
to pass this small identifier set and the packet shape only.
- Around line 67-87: The getObjectStoreConfig function currently reads
process.env directly; import the env export from app/env.server.ts and use
env.server to read the protocol-specific keys (e.g.,
env.server[`${prefix}BASE_URL`], env.server[`${prefix}ACCESS_KEY_ID`], etc.)
instead of process.env so the values go through the app's env schema/defaults;
keep the same validation (return undefined if required keys missing) and ensure
the service default ("s3") is applied when service is not set.
In `@apps/webapp/test/objectStore.test.ts`:
- Around line 5-10: The test suite imports functions
(downloadPacketFromObjectStore, uploadPacketToObjectStore, formatStorageUri,
parseStorageUri) from ~/v3/objectStore.server which itself imports ~/env.server
at module load, making tests order-dependent; refactor objectStore.server to not
read env at import time by exporting a factory (e.g., createObjectStoreClient or
initObjectStore) that accepts configuration/options (provider, credentials,
bucket, protocol) and update the tests to call that factory with explicit config
(or dynamically import the module after setting process.env) so tests pass
configuration via options rather than relying on env.server at module load;
ensure functions currently exported are either methods on the returned client or
thin wrappers that accept the config argument so tests can control provider
deterministically.
- Around line 54-56: The test saves a mutable reference with const originalEnv =
process.env so later restores reassign the already-mutated object and env leaks;
fix by cloning and restoring per test: in the test suite use beforeEach to save
a shallow copy (e.g., const savedEnv = { ...process.env } or
structuredClone(process.env)) and afterEach to restore it (replace process.env
with the saved copy or clear/assign keys back), and update the same pattern
around the other occurrence referenced (lines 256–259) so each test gets an
isolated process.env and no state leaks between tests; reference the originalEnv
constant and the afterAll/afterEach teardown code when making the change.
🪄 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: caea1762-24f5-4516-8045-952b2d93eb7f
📒 Files selected for processing (8)
apps/webapp/app/runEngine/services/batchTrigger.server.tsapps/webapp/app/v3/objectStore.server.tsapps/webapp/app/v3/services/batchTriggerV3.server.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsinternal-packages/testcontainers/src/utils.ts
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/runEngine/services/batchTrigger.server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/v3/services/triggerTaskV1.server.ts
- apps/webapp/app/v3/services/batchTriggerV3.server.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{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
**/*.{ts,tsx}: For apps and internal packages (apps/*,internal-packages/*), usepnpm run typecheck --filter <package>for verification, never usebuildas it proves almost nothing about correctness
Use testcontainers helpers (redisTest,postgresTest,containerTestfrom@internal/testcontainers) for integration tests with Redis and PostgreSQL instead of mocking
When writing Trigger.dev tasks, always import from@trigger.dev/sdk- never use@trigger.dev/sdk/v3or deprecatedclient.defineJob
Files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
**/*.{ts,tsx,js,jsx}: Use pnpm for package management in this monorepo (version 10.23.0) with Turborepo for orchestration - run commands from root withpnpm run
Add crumbs as you write code for debug tracing using//@Crumbscomments or `// `#region` `@crumbsblocks - they stay on the branch throughout development and are stripped viaagentcrumbs stripbefore merge
Files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.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/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.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/test/objectStore.test.tsapps/webapp/app/v3/objectStore.server.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
apps/webapp/test/objectStore.test.ts
apps/webapp/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Test files should only import classes and functions from
app/**/*.tsfiles and should not importenv.server.tsdirectly or indirectly; pass configuration through options instead
Files:
apps/webapp/test/objectStore.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/test/objectStore.test.tsapps/webapp/app/v3/objectStore.server.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 descriptivedescribeanditblocks
Tests should avoid mocks or stubs and use the helpers from@internal/testcontainerswhen Redis or Postgres are needed
Use vitest for running unit tests
**/*.test.{ts,tsx,js,jsx}: Use vitest exclusively for testing and never mock anything - use testcontainers instead for Redis/PostgreSQL
Place test files next to source files using naming convention:MyService.ts->MyService.test.ts
Files:
apps/webapp/test/objectStore.test.ts
apps/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
When modifying only server components (
apps/webapp/,apps/supervisor/, etc.) with no package changes, add a.server-changes/file instead of a changeset
Files:
apps/webapp/test/objectStore.test.tsapps/webapp/app/v3/objectStore.server.ts
apps/webapp/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
For testable code, never import
env.server.tsin test files. Pass configuration as options/constructor arguments instead
Files:
apps/webapp/test/objectStore.test.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/v3/objectStore.server.ts
apps/webapp/app/v3/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (
@internal/run-engine) and redis-worker, not legacy V1 engine code
Files:
apps/webapp/app/v3/objectStore.server.ts
apps/webapp/app/**/*.{ts,tsx,server.ts}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Access environment variables via
envexport fromapp/env.server.ts. Never useprocess.envdirectly
Files:
apps/webapp/app/v3/objectStore.server.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to apps/webapp/app/v3/**/*.{ts,tsx} : In the webapp v3 directory, only modify V2 code paths when encountering V1/V2 branching in services - all new work uses Run Engine 2.0 (`internal/run-engine`) and redis-worker, not legacy V1 engine code
📚 Learning: 2026-03-23T06:24:14.566Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to **/*.{ts,tsx} : Use testcontainers helpers (`redisTest`, `postgresTest`, `containerTest` from `internal/testcontainers`) for integration tests with Redis and PostgreSQL instead of mocking
Applied to files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.ts
📚 Learning: 2026-03-02T12:43:25.254Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: internal-packages/run-engine/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:25.254Z
Learning: Applies to internal-packages/run-engine/src/engine/tests/**/*.test.ts : Implement tests for RunEngine in `src/engine/tests/` using testcontainers for Redis and PostgreSQL containerization
Applied to files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.ts
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Tests should avoid mocks or stubs and use the helpers from `internal/testcontainers` when Redis or Postgres are needed
Applied to files:
internal-packages/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.ts
📚 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/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.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/testcontainers/src/utils.tsapps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.tsapps/webapp/app/v3/objectStore.server.ts
📚 Learning: 2026-03-23T06:24:14.566Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-23T06:24:14.566Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest exclusively for testing and never mock anything - use testcontainers instead for Redis/PostgreSQL
Applied to files:
apps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to **/*.{test,spec}.{ts,tsx} : Use vitest for all tests in the Trigger.dev repository
Applied to files:
apps/webapp/test/objectStore.test.ts
📚 Learning: 2026-01-15T10:48:02.687Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-15T10:48:02.687Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest for running unit tests
Applied to files:
apps/webapp/test/objectStore.test.ts
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/services/**/*.server.{ts,tsx} : Separate testable services from configuration files; follow the pattern of `realtimeClient.server.ts` (testable service) and `realtimeClientGlobal.server.ts` (configuration) in the webapp
Applied to files:
apps/webapp/test/objectStore.test.ts
📚 Learning: 2025-11-27T16:26:37.432Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-27T16:26:37.432Z
Learning: Applies to internal-packages/database/**/*.{ts,tsx} : Use Prisma for database interactions in internal-packages/database with PostgreSQL
Applied to files:
apps/webapp/test/objectStore.test.ts
📚 Learning: 2026-03-02T12:43:43.173Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: packages/redis-worker/CLAUDE.md:0-0
Timestamp: 2026-03-02T12:43:43.173Z
Learning: Applies to packages/redis-worker/**/redis-worker/**/*.{test,spec}.{ts,tsx} : Use testcontainers for Redis in test files for redis-worker
Applied to files:
apps/webapp/test/objectStore.test.tsinternal-packages/testcontainers/src/index.tsinternal-packages/testcontainers/src/minio.ts
📚 Learning: 2025-07-12T18:00:06.163Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 2264
File: apps/webapp/app/utils/searchParams.ts:16-18
Timestamp: 2025-07-12T18:00:06.163Z
Learning: The `objectToSearchParams` function in `apps/webapp/app/utils/searchParams.ts` is used to generate URL parameters from objects and is separate from code that parses incoming search parameters. Changes to this function only affect places where it's used to create URLs, not places that parse search parameters from external sources.
Applied to files:
apps/webapp/app/v3/objectStore.server.ts
📚 Learning: 2026-03-26T10:02:22.373Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 3254
File: apps/webapp/app/services/platformNotifications.server.ts:363-385
Timestamp: 2026-03-26T10:02:22.373Z
Learning: In `triggerdotdev/trigger.dev`, the `getNextCliNotification` fallback in `apps/webapp/app/services/platformNotifications.server.ts` intentionally uses `prisma.orgMember.findFirst` (single org) when no `projectRef` is provided. This is acceptable for v1 because the CLI (`dev` and `login` commands) always passes `projectRef` in normal usage, making the fallback a rare edge case. Do not flag the single-org fallback as a multi-org correctness bug in this file.
Applied to files:
apps/webapp/app/v3/objectStore.server.ts
🪛 GitHub Actions: 🤖 PR Checks
apps/webapp/test/objectStore.test.ts
[warning] 1-1: Vitest: The CJS build of Vite's Node API is deprecated. See https://vite.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.
apps/webapp/app/v3/objectStore.server.ts
[warning] 1-1: Vitest: The CJS build of Vite's Node API is deprecated. See https://vite.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.
| function getObjectStoreConfig(protocol?: string): ObjectStoreConfig | undefined { | ||
| if (protocol) { | ||
| // Named provider (e.g., OBJECT_STORE_S3_*) | ||
| const prefix = `OBJECT_STORE_${protocol.toUpperCase()}_`; | ||
| const baseUrl = process.env[`${prefix}BASE_URL`]; | ||
| const accessKeyId = process.env[`${prefix}ACCESS_KEY_ID`]; | ||
| const secretAccessKey = process.env[`${prefix}SECRET_ACCESS_KEY`]; | ||
| const region = process.env[`${prefix}REGION`]; | ||
| const service = process.env[`${prefix}SERVICE`]; | ||
|
|
||
| if (!baseUrl || !accessKeyId || !secretAccessKey) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return { | ||
| baseUrl, | ||
| accessKeyId, | ||
| secretAccessKey, | ||
| region, | ||
| service, | ||
| }; |
There was a problem hiding this comment.
Keep named-provider config behind env.server.
This branch reads process.env directly, so it bypasses the webapp env schema/defaults. Protocol-specific providers won't get the same boot-time validation/default handling as the legacy branch, including the new "s3" service default mentioned below.
As per coding guidelines "Access environment variables via env export from app/env.server.ts. Never use process.env directly".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/app/v3/objectStore.server.ts` around lines 67 - 87, The
getObjectStoreConfig function currently reads process.env directly; import the
env export from app/env.server.ts and use env.server to read the
protocol-specific keys (e.g., env.server[`${prefix}BASE_URL`],
env.server[`${prefix}ACCESS_KEY_ID`], etc.) instead of process.env so the values
go through the app's env schema/defaults; keep the same validation (return
undefined if required keys missing) and ensure the service default ("s3") is
applied when service is not set.
| import { | ||
| downloadPacketFromObjectStore, | ||
| formatStorageUri, | ||
| parseStorageUri, | ||
| uploadPacketToObjectStore, | ||
| } from "~/v3/objectStore.server"; |
There was a problem hiding this comment.
This suite is coupled to env.server initialization.
~/v3/objectStore.server imports ~/env.server at module load, but the tests mutate process.env later to switch providers. That makes the legacy/default-protocol paths order-dependent and can easily exercise stale config instead of the one the test just set.
As per coding guidelines "Test files should only import classes and functions from app/**/*.ts files and should not import env.server.ts directly or indirectly; pass configuration through options instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/test/objectStore.test.ts` around lines 5 - 10, The test suite
imports functions (downloadPacketFromObjectStore, uploadPacketToObjectStore,
formatStorageUri, parseStorageUri) from ~/v3/objectStore.server which itself
imports ~/env.server at module load, making tests order-dependent; refactor
objectStore.server to not read env at import time by exporting a factory (e.g.,
createObjectStoreClient or initObjectStore) that accepts configuration/options
(provider, credentials, bucket, protocol) and update the tests to call that
factory with explicit config (or dynamically import the module after setting
process.env) so tests pass configuration via options rather than relying on
env.server at module load; ensure functions currently exported are either
methods on the returned client or thin wrappers that accept the config argument
so tests can control provider deterministically.
apps/webapp/test/objectStore.test.ts
Outdated
| // Mock env module for testing | ||
| const originalEnv = process.env; | ||
|
|
There was a problem hiding this comment.
Clone and reset process.env per test.
const originalEnv = process.env stores the same mutable object, so the afterAll block just reassigns the already-mutated reference. These object-store vars can leak between cases and into later files in the same worker.
♻️ Suggested fix
-import { afterAll, describe, expect, it, vi } from "vitest";
+import { afterEach, describe, expect, it, vi } from "vitest";
@@
-const originalEnv = process.env;
+const originalEnv = { ...process.env };
@@
-afterAll(() => {
- process.env = originalEnv;
-});
+afterEach(() => {
+ for (const key of Object.keys(process.env)) {
+ delete process.env[key];
+ }
+
+ Object.assign(process.env, originalEnv);
+});Also applies to: 256-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/webapp/test/objectStore.test.ts` around lines 54 - 56, The test saves a
mutable reference with const originalEnv = process.env so later restores
reassign the already-mutated object and env leaks; fix by cloning and restoring
per test: in the test suite use beforeEach to save a shallow copy (e.g., const
savedEnv = { ...process.env } or structuredClone(process.env)) and afterEach to
restore it (replace process.env with the saved copy or clear/assign keys back),
and update the same pattern around the other occurrence referenced (lines
256–259) so each test gets an isolated process.env and no state leaks between
tests; reference the originalEnv constant and the afterAll/afterEach teardown
code when making the change.
| const { protocol, path } = parseStorageUri(filename); | ||
|
|
There was a problem hiding this comment.
🚩 Protocol-prefixed filenames in URL paths may encounter proxy normalization
When OBJECT_STORE_DEFAULT_PROTOCOL is set, stored filenames look like s3://run_abc/payload.json. The SDK API client at packages/core/src/v3/apiClient/index.ts:564 constructs URLs by embedding the filename directly in the path: ${baseUrl}/api/v1/packets/${filename}. This produces URLs containing :// in the path segment (e.g., /api/v1/packets/s3://run_abc/payload.json). While technically valid per RFC 3986, some reverse proxies (notably Nginx with default merge_slashes on) normalize double slashes in paths, potentially corrupting the :// to :/. This should work in most direct-to-server configurations and the test suite validates it against MinIO containers, but production deployments behind reverse proxies should be tested.
Was this helpful? React with 👍 or 👎 to provide feedback.
matt-aitken
force-pushed
the
object-storage-migration
branch
from
b03061c to
346c588
Compare
This allows seamless migration to different object storage.
- Fix objectStore test failures: replace manual PrismaClient(process.env.DATABASE_URL) with proper postgresAndMinioTest fixture (new combined testcontainer) - Fix undefined prefix in uploadDataToObjectStore: pathname now conditional - Remove duplicate MAXIMUM_DEV_QUEUE_SIZE/MAXIMUM_DEPLOYED_QUEUE_SIZE rows from docs
Delegate the object store client to aws4fetch (existing) of the official AWS S3Client for IAM login.
matt-aitken
force-pushed
the
object-storage-migration
branch
from
346c588 to
dd0891e
Compare
| export async function uploadPacketToObjectStore( | ||
| filename: string, | ||
| data: ReadableStream | string, | ||
| contentType: string, | ||
| environment: AuthenticatedEnvironment, | ||
| storageProtocol?: string | ||
| ): Promise<string> { | ||
| return await startActiveSpan("uploadPacketToObjectStore()", async (span) => { | ||
| const protocol = storageProtocol || env.OBJECT_STORE_DEFAULT_PROTOCOL; | ||
| const client = getObjectStoreClient(protocol); | ||
|
|
||
| if (!client) { | ||
| throw new Error(`Object store is not configured for protocol: ${protocol || "default"}`); | ||
| } | ||
|
|
||
| span.setAttributes({ | ||
| projectRef: environment.project.externalRef, | ||
| environmentSlug: environment.slug, | ||
| filename, | ||
| protocol: protocol || "default", | ||
| }); | ||
|
|
||
| const key = `packets/${environment.project.externalRef}/${environment.slug}/${filename}`; | ||
|
|
||
| logger.debug("Uploading to object store", { key, protocol: protocol || "default" }); | ||
|
|
||
| await client.putObject(key, data, contentType); | ||
|
|
||
| // Return filename with protocol prefix if specified | ||
| return formatStorageUri(filename, protocol); | ||
| }); |
There was a problem hiding this comment.
🚩 SDK-side uploads always use default provider, not named providers
The SDK's exportPacket function in packages/core/src/v3/utils/ioSerialization.ts:166-197 constructs filenames locally and uploads via presigned URLs from the api/v1/packets endpoint. These filenames never include protocol prefixes, meaning worker-uploaded data (task outputs) always goes to the default provider, not any named provider configured via OBJECT_STORE_DEFAULT_PROTOCOL. This is by design but has an important implication: during a migration from Provider A to Provider B, outputs still go to Provider A (default). Users must keep the default provider running as long as any active runs might produce outputs. The documentation at docs/self-hosting/env/webapp.mdx:252-253 mentions decommissioning the old provider after active runs complete, but doesn't explicitly call out that outputs always use the default provider.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.s3Client = new S3Client({ | ||
| endpoint: config.baseUrl, | ||
| forcePathStyle: true, | ||
| ...(config.region ? { region: config.region } : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚩 AwsSdkClient hardcodes forcePathStyle: true which limits AWS S3 compatibility
The AwsSdkClient at apps/webapp/app/v3/objectStoreClient.server.ts:82-87 hardcodes forcePathStyle: true in the S3Client config. AWS deprecated path-style URLs for S3 buckets created after September 2023. Users with newer AWS S3 buckets in IAM mode may encounter issues. This is a design trade-off (path-style is needed for MinIO and many S3-compatible services) rather than a bug, but it limits the IAM mode's compatibility with vanilla AWS S3. Consider making this configurable via an env var.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This allows seamless migration to different object storage.
Existing runs that have offloaded payloads/outputs will continue to use the default object store (configured using
OBJECT_STORE_*env vars).You can add additional stores by setting new env vars:
OBJECT_STORE_DEFAULT_PROTOCOLthis determines where new run large payloads will get stored.Example: