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.
| export function parseStorageUri(uri: string): ParsedStorageUri { | ||
| const match = uri.match(/^([a-z0-9]+):\/\/(.+)$/); | ||
| if (match) { | ||
| return { | ||
| protocol: match[1], | ||
| path: match[2], | ||
| }; | ||
| } | ||
| return { | ||
| protocol: undefined, | ||
| path: uri, | ||
| }; |
There was a problem hiding this comment.
Normalize protocol casing before persisting it.
getObjectStoreConfig() treats provider names case-insensitively via toUpperCase(), but parseStorageUri() only matches lowercase protocols. If a caller passes "S3" (or OBJECT_STORE_DEFAULT_PROTOCOL is uppercase), you'll store S3://... and later parse it as a legacy path on download. Normalize or validate the protocol once at the boundary before formatting/parsing it.
As per coding guidelines "Use zod for validation in packages/core and apps/webapp".
Also applies to: 44-48
🤖 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 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.
| 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.
| async putObject(key: string, body: ReadableStream | string, contentType: string): Promise<void> { | ||
| await this.s3Client.send( | ||
| new PutObjectCommand({ | ||
| Bucket: this.config.bucket, | ||
| Key: key, | ||
| Body: body as string, | ||
| ContentType: contentType, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 AwsSdkClient stores objects with doubled bucket prefix in key, breaking cross-client compatibility
The AwsSdkClient receives a key like packets/proj_ref/env_slug/file.json and passes it directly as the S3 Key with a separate Bucket parameter (e.g., "packets"). This stores the object at Bucket=packets, Key=packets/proj_ref/env_slug/file.json. In contrast, Aws4FetchClient puts the same key into the URL path (/packets/proj_ref/env_slug/file.json), where in path-style S3 addressing the first segment is the bucket, yielding Bucket=packets, Key=proj_ref/env_slug/file.json. This means the same logical key maps to different physical S3 objects depending on which client is used. While a single protocol consistently uses one client type, the generatePresignedRequest function (used by resources.packets.$environmentId.$.ts:37-50) constructs a presigned URL via the AwsSdkClient that points to packets/packets/..., creating a double-prefix path. If the deployment later switches from IAM mode to static credentials (or vice versa) for the same protocol, all previously stored data becomes inaccessible.
Prompt for agents
In apps/webapp/app/v3/objectStoreClient.server.ts, the AwsSdkClient receives full keys like 'packets/proj/env/file.json' but also uses a separate Bucket config (typically 'packets'). This creates a doubled prefix: Bucket='packets', Key='packets/proj/env/file.json'. To fix this, you need to either:
1. Strip the bucket prefix from the key before passing to S3 commands in AwsSdkClient (parse the key to extract the bucket and actual object key), or
2. Change the callers in objectStore.server.ts (uploadPacketToObjectStore, downloadPacketFromObjectStore, generatePresignedRequest, uploadDataToObjectStore) to NOT include the 'packets/' prefix in the key when the AwsSdkClient path is used, or
3. Have ObjectStoreClient.create() accept and store the bucket info, and have the IObjectStoreClient interface handle bucket-aware key construction internally.
The cleanest approach is option 3: let ObjectStoreClient expose a method that builds keys, and have AwsSdkClient strip the first path segment (the bucket name) from the key when it matches self.config.bucket. This ensures both client types address the same physical S3 object.
Was this helpful? React with 👍 or 👎 to provide feedback.
| new PutObjectCommand({ | ||
| Bucket: this.config.bucket, | ||
| Key: key, | ||
| Body: body as string, |
There was a problem hiding this comment.
🟡 AwsSdkClient.putObject silently casts ReadableStream to string, will fail at runtime for stream uploads
The putObject method accepts body: ReadableStream | string but casts it to string via Body: body as string at line 91. The AWS SDK v3 PutObjectCommand Body field accepts string | Uint8Array | Buffer | Readable | ReadableStream | Blob, so the cast is unnecessary. However, if a ReadableStream is passed, the as string cast tricks TypeScript but at runtime the SDK receives a ReadableStream. While the SDK can handle ReadableStreams, the explicit as string cast is incorrect and could cause subtle issues if the SDK internally checks typeof body === 'string' for content-length calculation or encoding decisions. The uploadPacketToObjectStore function signature accepts ReadableStream | string (apps/webapp/app/v3/objectStore.server.ts:126), so a ReadableStream body reaching this code path is plausible.
| Body: body as string, | |
| Body: body, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { protocol, path } = parseStorageUri(filename); | ||
|
|
There was a problem hiding this comment.
🚩 Protocol prefix in stored data could cause issues with SDK packet URL construction
When OBJECT_STORE_DEFAULT_PROTOCOL=s3, payload data stored in the DB will be s3://run_abc/payload.json. The SDK's conditionallyImportPacket (packages/core/src/v3/utils/ioSerialization.ts:252-263) calls $client.getPayloadUrl(packet.data), which hits api.v1.packets.$.ts with the filename as a URL path segment. The :// characters in s3://run_abc/payload.json may need URL encoding when used in the path. Remix's route parameter decoding should handle this transparently (the * splat param captures and decodes), and parseStorageUri at objectStore.server.ts:250 correctly parses it back. But this depends on how the SDK constructs the API URL — if it doesn't URL-encode the filename, some HTTP clients or proxies might interpret :// as a protocol.
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
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: