feat(deployments): --native-build-server support for the deploy command
#2702
Conversation
🦋 Changeset detectedLatest commit: f0cd177 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds artifact upload support using S3 presigned-posts (new ARTIFACTS_* env vars, ArtifactsService, and a POST API route). Introduces S2-based deployment event streaming with Redis-backed token caching and new DeploymentService APIs for creating streams, appending events, and issuing access tokens. Extends deployment lifecycle flows (initialize, finalize, fail, timeout, index-failed) to emit events and to support native builds (enqueueing builds, returning eventStream). Adds CLI native-build-server features (context archiving, upload, streaming, --native-build-server/--detach/--plain options), plain-mode logging/spinners, and new core schemas for artifact endpoints, initialize-discriminated union, and deployment events. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
myftija
force-pushed
the
cli-deploy-with-native-builders
branch
from
f55aad5 to
cf46ed4
Compare
myftija
changed the title
--native-build-server support for the deploy command
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli-v3/src/utilities/windows.ts (1)
89-99: Windows implementation ignores the options parameter; the suggested fix is incomplete.The
ballmerSpinnerfunction doesn't accept options, and critically, on line 103 it's called without passingoptionswhilewrappedClackSpinnerreceives them. This creates inconsistent behavior where Windows users won't receivecancelMessagehandling.The suggested diff updates
ballmerSpinner's signature but misses updating the call site on line 103. Both changes are needed:-const ballmerSpinner = () => ({ +const ballmerSpinner = (options: { cancelMessage?: string } = {}) => ({ start: (msg?: string): void => { log.step(msg ?? ""); }, stop: (msg?: string, code?: number): void => { - log.message(msg ?? ""); + log.message(msg ?? options.cancelMessage ?? ""); }, message: (msg?: string): void => { log.message(msg ?? ""); }, }); -export const spinner = (options: { cancelMessage?: string } = {}) => - isWindows ? ballmerSpinner() : wrappedClackSpinner(options); +export const spinner = (options: { cancelMessage?: string } = {}) => + isWindows ? ballmerSpinner(options) : wrappedClackSpinner(options);packages/cli-v3/src/commands/deploy.ts (1)
195-201: The--dry-runflag is indeed ignored when--native-build-serveris enabledThe control flow confirms the issue: when
options.nativeBuildServeris true (lines 300–310), the function callshandleNativeBuildServerDeployand returns immediately, bypassing the dry-run check at line 353. ThehandleNativeBuildServerDeployfunction contains no dry-run logic whatsoever and unconditionally creates artifacts, uploads files, and callsinitializeDeployment.This creates a semantic mismatch where
deploy --native-build-server --dry-runperforms a real deployment instead of a dry-run, contradicting the documented behavior of--dry-run.Suggested fixes remain valid:
- Implement true native dry-run (prepare archive, show size/path, skip artifact upload and deployment init), or
- Reject the combination explicitly with a clear error message
🧹 Nitpick comments (10)
apps/webapp/app/services/platform.v3.server.ts (1)
532-555: Consider adding input validation.The
enqueueBuildfunction acceptsartifactKeyandoptionswithout validation. Consider adding checks to ensure:
artifactKeyis non-emptydeploymentIdis in expected format- Optional parameters are valid when provided
This would provide early failure detection before calling the platform client.
apps/webapp/app/v3/services/deploymentIndexFailed.server.ts (1)
4-5: Failure event logging integration looks correct; consider tightening selection and log noiseThe added
environment.projectinclude paired withDeploymentService.appendToEventLog(...)wires this failure path into the deployment event log correctly without affecting the main control flow. UsingfailedDeploymentfor the shortCode and the originaldeployment.environment.projectforexternalRefis sound.Two minor refinements to consider:
- You can
select: { externalRef: true }instead ofproject: truein the include to avoid fetching unused columns on every index failure.appendToEventLogreturns ans2_is_disablederror when S2 is off; with.orTeethis will be logged as an error on every failure. If S2-disabled is expected in some environments, you may want to downgrade that specific case to debug/info or use.orElseto swallow it while still logging unexpected failures.Also applies to: 35-41, 77-90
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
1-2: Finalization event logging is correctly integrated and non‑disruptiveThe added
DeploymentServiceusage to append a"finalized" -> "succeeded"event after marking the deployment asDEPLOYEDis consistent with the other lifecycle paths. UsingauthenticatedEnv.projectfor the project scope andfinalizedDeploymentfor the shortCode is correct, and.orTeekeeps failures from affecting the main finalize flow.As with other call sites, if you expect S2 to be disabled in some environments, you may eventually want to treat the
"s2_is_disabled"error as non-error logging to avoid noisy logs, but the current behavior is safe.Also applies to: 12-12, 81-93
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
14-16: Native build and event stream initialization flow looks solid; a couple of small hardening opportunitiesThe changes here nicely integrate native builds and S2 event streams:
- Skipping
createRemoteImageBuildwhenpayload.initialStatus === "PENDING"orpayload.isNativeBuildavoids unnecessary Depot builds.- The new
initialStatuslogic preserves existing behavior for non-native deployments while defaulting native builds toPENDINGwhen the caller doesn’t override it.- Creating the S2 stream + access token up front and returning
eventStream(orundefinedon failure) gives the CLI an immediate handle for streaming logs without coupling it to S2 availability.- The native build path that calls
enqueueBuild(...), logs failures, attempts cancellation, and then surfaces a generic error keeps the database state consistent even when platform enqueue fails.Two small robustness tweaks you might consider:
Enforce
artifactKeyfor native builds at the service boundaryRight now, a missing or empty
payload.artifactKeyforisNativeBuild === truewill only surface as a “Failed to enqueue build” after a platform call and subsequent cancellation. Adding an explicit validation up front would yield a clearer, more actionable error:if (payload.isNativeBuild && !payload.artifactKey) { throw new ServiceValidationError("artifactKey is required for native builds"); }Align the comment with actual
initialStatusbehaviorThe comment says “Native builds always start in the
PENDINGstatus”, but the code keeps an explicitpayload.initialStatusif provided. Either tightening the code to always forcePENDINGwhenisNativeBuildis true, or rephrasing the comment (e.g. “Native builds default toPENDINGwhen an initialStatus isn’t provided”) would avoid confusion for future readers.Neither of these is blocking, but they’d make the behavior more predictable and easier to reason about.
Also applies to: 95-99, 141-177, 189-191, 223-256, 261-262
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
11-22: Timed‑out deployment event logging looks good; consider guarding for missing projectUsing the updated
timedOutDeploymenttogether with the deployment service to append a finalized"timed_out"event before enqueueing alerts keeps the timeout path consistent with the rest of the deployment event logging.If there’s any chance of legacy rows without
environment/project, you may want a quick guard like:if (!deployment.environment?.project) { logger.error("Deployment missing environment.project, skipping event log append", { deploymentId: deployment.id, }); } else { await deploymentService .appendToEventLog(deployment.environment.project, timedOutDeployment, [/* … */]) .orTee((error) => { logger.error("Failed to append timed out deployment event to event log", { error }); }); }to avoid a runtime throw preventing alerts from being enqueued.
Also applies to: 37-61
packages/cli-v3/src/deploy/archiveContext.ts (1)
51-63: Context archiving logic is solid; a couple of minor resilience/perf tweaksThe overall flow (default ignores +
.gitignore→ glob →ignorere‑filter) looks good and should behave as expected.Two small, optional improvements:
- Graceful failure for unreadable
.gitignore– instead of throwing, you could log and proceed without it so a bad.gitignoredoesn’t block deployments.- Avoid sync I/O in
getArchiveSize– since you’re already async, using the asyncstatAPI keeps the event loop non‑blocking.For example:
-import { readFile } from "node:fs/promises"; +import { readFile, stat } from "node:fs/promises"; @@ const [error, content] = await tryCatch(readFile(gitignorePath, "utf-8")); if (error) { - throw new Error(`Failed to read .gitignore at ${gitignorePath}: ${error.message}`); + logger.debug("Failed to read .gitignore, continuing without it", { + gitignorePath, + error, + }); + return ""; } @@ export async function getArchiveSize(archivePath: string): Promise<number> { - const { statSync } = await import("node:fs"); - return statSync(archivePath).size; + const stats = await stat(archivePath); + return stats.size; }Also applies to: 151-154
apps/webapp/app/v3/services/artifacts.server.ts (1)
35-61: Remove or use the unusedcontentLengthparameter increatePresignedPost
createPresignedPosttakes acontentLengthargument but never uses it; the only enforced limit issizeLimitvia the"content-length-range"condition. That’s slightly confusing for readers and callers.If you don’t plan to enforce an exact size server‑side, I’d drop the parameter for now:
- public createArtifact( + public createArtifact( type: "deployment_context", authenticatedEnv: AuthenticatedEnvironment, contentLength?: number ) { @@ - return this.createPresignedPost(key, limit, contentLength).map((result) => ({ + return this.createPresignedPost(key, limit).map((result) => ({ @@ - private createPresignedPost(key: string, sizeLimit: number, contentLength?: number) { + private createPresignedPost(key: string, sizeLimit: number) {You can always reintroduce a
contentLengthconstraint later if you decide to enforce it at the presign level.Also applies to: 63-85
packages/cli-v3/src/commands/deploy.ts (1)
875-999: Native build streaming flow is nice; consider avoidingprocess.exitand ensure Node globals are availableThe native path that uploads the context archive, then streams build logs/events from S2 into a
taskLoggives a good UX and matches the newDeploymentEventmodel.Two optional considerations:
- Node runtime expectations – this helper relies on global
fetch,FormData,Blob,AbortController, andTextDecoder. Make sure the CLI’s supported Node version range guarantees these are available, or document that requirement, otherwise some users may hit runtime ReferenceErrors.- Process termination inside the helper – rather than calling
process.exit(0)on success/non‑streaming paths, you could justreturnand let the surrounding command/telemetry wrapper decide when to exit. That tends to make the function easier to test and reuse.The small
DeploymentEventFromStringhelper at the bottom is a clean way to couple JSON parsing and schema validation before handling events.Also applies to: 1001-1112, 1254-1264
apps/webapp/app/v3/services/deployment.server.ts (2)
307-423: S2 event helpers and token cache: good structure with minor reuse opportunitiesThe trio
appendToEventLog/createEventStream/getEventStreamAccessTokenis well‑structured:
- Consistent stream naming (
projects/${project.externalRef}/deployments/${deployment.shortCode}).- Early guard when S2 is disabled.
- Token cache flow that gracefully falls back to issuing a new token and tolerates cache write failures while still returning a usable token.
Two minor maintainability tweaks you could consider:
- Factor out the stream name construction
You currently repeat the same pattern in multiple places (append, create, and implicitly in the token scope). Extracting a helper keeps these in sync:
+ private deploymentStreamName( + project: Pick<Project, "externalRef">, + deploymentShortCode: string + ) { + return `projects/${project.externalRef}/deployments/${deploymentShortCode}`; + } + public appendToEventLog( @@ - const stream = basin.stream( - `projects/${project.externalRef}/deployments/${deployment.shortCode}` - ); + const stream = basin.stream( + this.deploymentStreamName(project, deployment.shortCode) + ); @@ public createEventStream( @@ - stream: `projects/${project.externalRef}/deployments/${deployment.shortCode}`, + stream: this.deploymentStreamName(project, deployment.shortCode), @@ - streams: { - prefix: `projects/${project.externalRef}/deployments/`, - }, + streams: { + prefix: `projects/${project.externalRef}/deployments/`, + },(You may also want a separate helper for the stream prefix if it ever diverges from the full name.)
- Optional: log cache read failures as well
Right now, any Redis
geterror is silently treated like a cache miss and onlysetexerrors are logged. If you care about diagnosing cache health, you might want a lightweight log in thegetTokenFromCacheerror mapper (still falling back to token issuance afterward).These are nice‑to‑haves; the core behavior and error handling look solid.
436-445: ExtendedgetDeploymentselect to includeshortCodeand project refAdding
shortCodeandenvironment.project.externalRefto the sharedgetDeploymentselection is necessary for the new S2 event‑logging paths and keeps the lookup centralized.The trade‑off is a slightly heavier query (extra join) for callers that only need
id/status/imageReference, but that’s probably acceptable unless this path is very hot. If you see this becoming a perf hotspot, a follow‑up could split out a lighter‑weight helper for those simpler use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.changeset/config.json(1 hunks).changeset/proud-birds-change.md(1 hunks)apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/api.v1.artifacts.ts(1 hunks)apps/webapp/app/routes/api.v1.deployments.ts(2 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/v3/services/artifacts.server.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(8 hunks)apps/webapp/app/v3/services/deploymentIndexFailed.server.ts(3 hunks)apps/webapp/app/v3/services/failDeployment.server.ts(2 hunks)apps/webapp/app/v3/services/finalizeDeployment.server.ts(3 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(5 hunks)apps/webapp/app/v3/services/timeoutDeployment.server.ts(4 hunks)apps/webapp/package.json(2 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)packages/cli-v3/package.json(4 hunks)packages/cli-v3/src/apiClient.ts(2 hunks)packages/cli-v3/src/cli/common.ts(1 hunks)packages/cli-v3/src/commands/deploy.ts(8 hunks)packages/cli-v3/src/commands/workers/build.ts(1 hunks)packages/cli-v3/src/deploy/archiveContext.ts(1 hunks)packages/cli-v3/src/utilities/windows.ts(3 hunks)packages/core/src/v3/schemas/api.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
apps/webapp/package.json
📚 Learning: 2025-11-14T19:24:39.536Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
Applied to files:
.changeset/proud-birds-change.md
🧬 Code graph analysis (12)
apps/webapp/app/services/platform.v3.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
enqueueBuild(289-305)
apps/webapp/app/routes/api.v1.artifacts.ts (3)
apps/webapp/app/services/apiAuth.server.ts (1)
authenticateRequest(379-441)packages/core/src/v3/schemas/api.ts (4)
CreateArtifactRequestBody(417-421)CreateArtifactRequestBody(423-423)CreateArtifactResponseBody(425-430)CreateArtifactResponseBody(432-432)apps/webapp/app/v3/services/artifacts.server.ts (1)
ArtifactsService(32-86)
apps/webapp/app/v3/services/failDeployment.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)
packages/cli-v3/src/apiClient.ts (2)
packages/core/src/v3/schemas/api.ts (4)
CreateArtifactRequestBody(417-421)CreateArtifactRequestBody(423-423)CreateArtifactResponseBody(425-430)CreateArtifactResponseBody(432-432)packages/core/src/v3/apiClient/core.ts (1)
wrapZodFetch(727-767)
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)
apps/webapp/app/v3/services/artifacts.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1252-1252)
apps/webapp/app/v3/services/deploymentIndexFailed.server.ts (2)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)
packages/core/src/v3/schemas/api.ts (1)
packages/core/src/v3/schemas/common.ts (2)
GitMeta(239-253)GitMeta(255-255)
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)
apps/webapp/app/v3/services/deployment.server.ts (3)
apps/webapp/app/env.server.ts (1)
env(1252-1252)apps/webapp/app/services/platform.v3.server.ts (1)
enqueueBuild(532-555)packages/core/src/v3/schemas/api.ts (2)
DeploymentEvent(600-603)DeploymentEvent(605-605)
packages/cli-v3/src/commands/deploy.ts (8)
packages/core/src/v3/schemas/common.ts (2)
GitMeta(239-253)GitMeta(255-255)packages/cli-v3/src/utilities/windows.ts (1)
spinner(103-104)packages/cli-v3/src/deploy/archiveContext.ts (2)
createContextArchive(91-149)getArchiveSize(151-154)packages/cli-v3/src/utilities/fileSystem.ts (1)
readFile(50-52)packages/cli-v3/src/utilities/githubActions.ts (1)
setGithubActionsOutputAndEnvVars(3-27)packages/cli-v3/src/utilities/cliOutput.ts (5)
isLinksSupported(7-7)cliLink(140-145)chalkGrey(20-22)chalkWarning(28-30)chalkError(24-26)packages/core/src/v3/schemas/api.ts (4)
DeploymentFinalizedEvent(592-598)DeploymentFinalizedEvent(607-607)DeploymentEvent(600-603)DeploymentEvent(605-605)packages/cli-v3/src/cli/common.ts (1)
OutroCommandError(35-35)
🪛 Biome (2.1.2)
packages/cli-v3/src/commands/deploy.ts
[error] 1140-1164: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
packages/cli-v3/src/commands/workers/build.ts (1)
242-242: LGTM! Correct discriminator for traditional build path.The addition of
isNativeBuild: falseappropriately identifies this as the traditional worker build workflow, distinguishing it from the new native build path being introduced in the PR.packages/cli-v3/src/cli/common.ts (1)
68-73: OutroCommandError now exits with code 1 – confirm impact on callersThe added
process.exit(1);bringsOutroCommandErrorin line withBundleError, but it changes behavior: any command path that previously threwOutroCommandErrorwill now terminate the process with a non‑zero exit code instead of behaving like a soft cancel. If any scripts/tools wrap these commands and treat “cancelled by user” as a success (exit code 0), this will be a breaking change.If the intention is to treat cancellations as hard failures, this looks good. Otherwise, consider either:
- Using
process.exitCode = 1(to keep behavior but allow outer handlers to decide), or- Leaving cancellation as a
SkipCommandErrorpath that does not force a non‑zero exit.packages/cli-v3/src/utilities/windows.ts (2)
40-50: LGTM! Options properly forwarded to clackSpinner.The signature update correctly accepts the options object and forwards it to the underlying
clackSpinner. The implementation for non-Windows platforms is correct.
103-104: LGTM! Backward-compatible API enhancement.The exported
spinnerfunction correctly accepts an optionaloptionsparameter with a sensible default, maintaining backward compatibility while enabling the new cancel message functionality. The platform-specific routing is correct.internal-packages/database/prisma/schema.prisma (1)
1749-1749: LGTM! Whitespace formatting adjustment.The additional whitespace aligns the field formatting with surrounding model fields without changing the schema definition.
packages/cli-v3/package.json (2)
95-95: LGTM! New dependencies align with PR objectives.The added dependencies support the native build server features:
@s2-dev/streamstore: Enables S2-based event streaming for deploymentsignore: Supports .gitignore-style file filtering for deployment archivestar: Required for creating deployment context archivesAlso applies to: 116-116, 140-140
84-84: Verify that @clack/prompts 1.0.0-alpha.6 is compatible with your CLI usage.Your update to 1.0.0-alpha.6 is confirmed as an alpha prerelease with potential for breaking API changes. Before adopting this version, ensure that:
- The alpha version does not introduce breaking changes to the APIs your CLI depends on
- Your current usage of
@clack/promptsfunctions remains compatible- You are comfortable accepting ongoing instability until the stable v1.0.0 release
apps/webapp/package.json (2)
33-33: LGTM! AWS SDK additions support artifact storage.The AWS SDK v3 modules are properly versioned and support the new S3-based artifact upload functionality.
Also applies to: 36-38
121-121: Verify prerelease platform version for production.The
@trigger.dev/platformdependency uses a prerelease version0.0.0-prerelease-20251121183112. Ensure this version is stable and intended for production deployment, or plan to update to a stable release before merging.apps/webapp/app/env.server.ts (1)
349-353: LGTM! Artifact storage configuration follows existing patterns.The new
ARTIFACTS_OBJECT_STORE_*environment variables mirror the existingOBJECT_STORE_*configuration pattern. MakingBUCKETrequired while keeping credentials optional allows for flexible authentication methods (default credentials, IAM roles, etc.)..changeset/proud-birds-change.md (1)
1-6: LGTM! Changeset clearly describes the feature.The changeset appropriately documents the native build server support addition at the patch level.
apps/webapp/app/v3/services/failDeployment.server.ts (1)
54-67: LGTM! Proper integration of deployment event logging.The deployment failure event is correctly appended to the S2 event log with appropriate error handling. The
orTeepattern ensures that event logging failures don't interrupt the deployment failure workflow..changeset/config.json (1)
3-3: Configuration value is valid and standard.The changelog generator "@changesets/cli/changelog" is the built-in/default changelog generator used by Changesets, so this is the correct standard configuration for your
.changeset/config.json.apps/webapp/app/routes/api.v1.deployments.ts (1)
40-52: eventStream propagation is consistent with the updated service and API schemaWiring
eventStreamthrough fromInitializeDeploymentService.callinto theInitializeDeploymentResponseBodyresponse looks consistent with the new initialization contract, and callers will simply seeeventStreamomitted/undefined when S2 is disabled or stream creation fails.packages/cli-v3/src/apiClient.ts (1)
3-4: CLI artifact client method aligns with existing patterns
createArtifactmirrors the other client methods well: it validatesaccessToken, reusesgetHeaders()(including branch), targets/api/v1/artifacts, and wraps the response withCreateArtifactResponseBodyviawrapZodFetch. This should plug cleanly into the new artifact flow.Also applies to: 365-375
packages/core/src/v3/schemas/api.ts (1)
417-432: New artifact and deployment event schemas look consistent with the native build flowThe added schemas for artifacts (
CreateArtifact*), the extendedInitializeDeployment*(with theisNativeBuilddiscriminator and optional native‑build fields), and theDeploymentEventunion (log + finalized with an open‑endedresult) line up well with the new artifact + streaming behavior in the CLI and services. I don’t see issues with these API shapes from a compatibility or expressiveness standpoint.Also applies to: 434-487, 582-607
packages/cli-v3/src/commands/deploy.ts (1)
62-82: Native build server flag wiring and branching look correctThe new
nativeBuildServerflag is plumbed through Zod and commander, and the early branch tohandleNativeBuildServerDeploykeeps the existing build/deploy path intact for users who don’t opt in. Explicitly sendingisNativeBuild: falseon the non‑native path also fits the updated schema and makes the behavior of older CLIs clearer on the server side.Also applies to: 174-179, 300-310, 358-367
apps/webapp/app/v3/services/deployment.server.ts (4)
3-5: Module‑level S2 and Redis wiring looks consistentThe new neverthrow imports plus S2 client and Redis cache initialization are straightforward and match existing env/Redis patterns in the app. Centralizing these at module scope for reuse is reasonable; no issues from my side here.
Also applies to: 10-12, 14-23
126-135: UsinggetDeploymentinprogressDeploymentimproves consistencySwitching
progressDeploymentto go through the sharedgetDeploymenthelper (instead of inlining a Prisma query) is a nice cleanup and keeps selection/lookup logic centralized. The control flow and timeout extension behavior remain unchanged.
152-156: Cancellation + finalization event logging behavior and log levelThe enriched types (shortCode + environment.project.externalRef) and the new “finalized/canceled” event append make sense, and I like that any S2 failures are tolerated so cancellation still completes.
One thing to consider: in environments where S2 is intentionally disabled,
appendToEventLogwill fail withs2_is_disabled, and the.orElsewill log this as an error on every cancellation. That can get noisy and misleading when S2 is off by design. You might want to downgrade this tologger.debug/logger.infoor branch on the error type:- .orElse((error) => { - logger.error("Failed to append event to deployment event log", { error }); - return okAsync(deployment); - }) + .orElse((error) => { + const level = + error.type === "s2_is_disabled" ? "info" : "error"; + logger[level]("Failed to append event to deployment event log", { error }); + return okAsync(deployment); + })[ suggest_optional_refactor ]
Also applies to: 167-171, 194-195, 203-221
289-305: Clarify semantics when platformenqueueBuildis unavailableThis wrapper cleanly maps platform errors into a
failed_to_enqueue_buildresult, which is good. However, the underlyingplatform.v3.enqueueBuildreturnsundefined(without throwing) whenclientis not configured, which this wrapper currently treats as a successfulOk(undefined).If this method is used in contexts where a remote build is required (e.g.
--native-build-serverin cloud), consider treating a falsy result as an error so we don’t silently no‑op:return fromPromise( - enqueueBuild(authenticatedEnv.projectId, deployment.friendlyId, artifactKey, options), - (error) => ({ + enqueueBuild(authenticatedEnv.projectId, deployment.friendlyId, artifactKey, options), + (error) => ({ type: "failed_to_enqueue_build" as const, cause: error, }) - ); + ).andThen((result) => { + if (!result) { + return errAsync({ type: "failed_to_enqueue_build" as const, cause: "no_client" }); + } + return okAsync(undefined); + });If the current “no client == benign no‑op” behavior is intentional for self‑hosted setups, it’d be good to document that expectation at the call sites.
[ suggest_recommended_refactor ]
myftija
force-pushed
the
cli-deploy-with-native-builders
branch
from
cf46ed4 to
91358c5
Compare
Spent a lot of time on it, but could not get it to work reliably. It clears lines that it should not clear when maininging the compact n-lines view during the log stream. Replaced it with a normal log instead for now.
myftija
force-pushed
the
cli-deploy-with-native-builders
branch
from
91358c5 to
81f63ba
Compare
Avoid confusion with self-hosters.
| "@trigger.dev/database": "workspace:*", | ||
| "@trigger.dev/otlp-importer": "workspace:*", | ||
| "@trigger.dev/platform": "1.0.20", | ||
| "@trigger.dev/platform": "0.0.0-prerelease-20251121183112", |
There was a problem hiding this comment.
update to the actual release before merging
| @@ -1,11 +1,6 @@ | |||
| { | |||
| "$schema": "https://unpkg.com/@changesets/[email protected]/schema.json", | |||
| "changelog": [ | |||
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "trigger.dev": patch | |||
There was a problem hiding this comment.
could also be a minor version bump
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli-v3/src/commands/deploy.ts (1)
62-83:--dry-runis ignored for--native-build-server, which breaks the documented contractWhen
options.nativeBuildServeris true,_deployCommandroutes intohandleNativeBuildServerDeployand never reaches the existingoptions.dryRuncheck (Lines 360–363). In the native path,handleNativeBuildServerDeployalways creates an artifact, uploads it, initializes the deployment, and enqueues the build; there is no special handling fordryRun.That means running:
trigger.dev deploy --native-build-server --dry-runwill still create and start a real deployment, which contradicts the current description of
--dry-run(“This will not actually deploy the project”).I’d recommend either:
- Short‑term: explicitly reject this combination up-front, e.g.:
async function _deployCommand(dir: string, options: DeployCommandOptions) { + if (options.nativeBuildServer && options.dryRun) { + throw new Error("The --dry-run option is not supported with --native-build-server yet."); + }
- Or, longer‑term: add
dryRunhandling intohandleNativeBuildServerDeployso that it only builds the local archive (and maybe logs its path/size) without creating the artifact, callinginitializeDeployment, or streaming events.Also applies to: 175-186, 203-205, 307-317, 360-363, 867-1007
♻️ Duplicate comments (1)
apps/webapp/app/routes/api.v1.artifacts.ts (1)
27-32: Handle JSON parse failures explicitly instead of defaulting to{}Right now:
const [, rawBody] = await tryCatch(request.json()); const body = CreateArtifactRequestBody.safeParse(rawBody ?? {});If
request.json()throws (malformed JSON, wrong content type, empty body),rawBodyends upundefined,safeParse({})may still succeed due to schema defaults, and you’ll create an artifact for an invalid request instead of returning a 4xx.Consider short‑circuiting on JSON parse errors and only running the schema validation when parsing succeeds, e.g.:
- const [, rawBody] = await tryCatch(request.json()); - const body = CreateArtifactRequestBody.safeParse(rawBody ?? {}); + const [bodyError, rawBody] = await tryCatch(request.json()); + + if (bodyError) { + logger.warn("Invalid JSON in artifact request body", { + url: request.url, + error: bodyError.message ?? String(bodyError), + }); + return json({ error: "Invalid JSON body" }, { status: 400 }); + } + + const body = CreateArtifactRequestBody.safeParse(rawBody ?? {});Also, please double‑check that
tryCatchis actually exported from@trigger.dev/core/v3for the version you’re using—other parts of the codebase typically import it from@trigger.dev/coreor related utils, so aligning this import may avoid surprises.#!/bin/bash # Verify tryCatch import patterns and availability # See how tryCatch is imported elsewhere for consistency rg "import.*tryCatch" -n # Confirm the resolved d.ts for @trigger.dev/core/v3 (path may vary) fd "core.*.d.ts" node_modules | head
🧹 Nitpick comments (7)
packages/cli-v3/src/cli/common.ts (1)
68-82: Consider usingprocess.exitCode = 1instead of hardprocess.exit(1)in wrapperCalling
process.exit(1)insidewrapCommandActionmakes this helper much harder to reuse or unit-test (any caller that doesn’t stubprocess.exitwill have the process terminated), and the subsequentthrow ebecomes effectively dead in real CLI runs. Consider mirroringhandleTelemetryand just settingprocess.exitCode = 1, leaving actual process termination to the top-level entrypoint/Commander.packages/cli-v3/src/utilities/windows.ts (1)
40-51: Spinner options propagation looks good; you may want a reusable options typePassing an options object through to
clackSpinneris a nice improvement and the default{}keeps this backward compatible for existingspinner()call sites. If you expect to support more spinner options later, consider defining a sharedSpinnerOptionstype (and using it for bothwrappedClackSpinnerandspinner) that can be expanded without having to touch all call sites again.Also applies to: 103-104
packages/core/src/v3/schemas/api.ts (2)
455-487: InitializeDeploymentRequestBody native-build union is well-shaped; consider clarifying docsThe intersection of the existing base object with a discriminated union on
isNativeBuild(defaulting tofalseviapreprocess) gives you:
- Backwards compatibility for legacy callers that don’t send
isNativeBuildat all (they land in theisNativeBuild: falsebranch).- A stricter native-build variant that requires
skipPromotionandartifactKey, with optionalconfigFilePath.This is a solid way to evolve the API. I’d just suggest adding a brief JSDoc comment explaining that
isNativeBuilddefaults tofalsewhen missing so future readers don’t accidentally remove thepreprocessand break old CLIs.
582-607: Deployment event schemas look good;anyStringhelper could be simplifiedThe
DeploymentLogEvent/DeploymentFinalizedEvent/DeploymentEventdiscriminated union is a clean way to model deployment log vs finalization events, and the optional+defaultedlevelon log events is a nice touch. TheanyStringcustom schema forresultis functionally equivalent to just allowing any string, though, so:result: z.enum(["succeeded", "failed", "timed_out", "canceled"]).or(z.string())would be simpler and convey the same runtime behavior and effective type.
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
14-15: Native-build initialization & S2 event stream integration look solid; consider exposing enqueue error detail
- Skipping
externalBuildDatawheninitialStatus === "PENDING"orpayload.isNativeBuildand computinginitialStatusaspayload.initialStatus ?? (payload.isNativeBuild ? "PENDING" : "BUILDING")preserves existing behavior while cleanly separating native builds into a queued state.- Creating the S2 event stream + access token via
DeploymentServiceand treating any failure as non-fatal (logging and returningeventStream: undefined) aligns well with S2 being optional; non-S2 environments will still proceed with deployments. Based on learnings, this matches the “S2 may be disabled” constraint.- The native-build path that enqueues a build and cancels the deployment on failure is robust, and the neverthrow chaining correctly preserves the original enqueue error even if cancellation itself fails.
If you want more actionable feedback to callers when enqueue fails, consider throwing with some of the structured error details (e.g.,
error.typeand/or a human-readable message) instead of a genericError("Failed to enqueue build"), while still keeping the full cause in logs.Also applies to: 95-98, 141-177, 189-191, 223-256, 258-262
apps/webapp/app/v3/services/artifacts.server.ts (1)
1-92: ArtifactsService is well-structured; minor refinements for size check and expiresAt shapeThe presigned-post flow and key layout look good, with sensible size limits and error handling. Two small tweaks you might consider:
- Avoid truthy check on
contentLength
Using a truthy check will skip validation forcontentLength = 0. It’s not wrong, but a more explicit check is clearer and future-proof:- if (contentLength && contentLength > limit) { + if (contentLength !== undefined && contentLength > limit) { return errAsync({ type: "artifact_size_exceeds_limit" as const, contentLength, sizeLimit: limit, }); }
- Align
expiresAtwith the public schema (string vs Date)
CreateArtifactResponseBodyexpectsexpiresAtas an ISO string; you currently attach aDateand rely on JSON serialization at the route layer. Making it a string here avoids surprises for internal consumers before serialization:).map((result) => ({ ...result, - expiresAt, + expiresAt: expiresAt.toISOString(), }));Neither change is strictly required for correctness with the current route, but they make the service’s contract clearer.
apps/webapp/app/v3/services/deployment.server.ts (1)
3-24: DeploymentService S2 integration and enqueueBuild helper look correct; consider richer logging context
- Centralizing deployment lookups via
getDeploymentand reusing it inprogressDeploymentandcancelDeploymentis a nice cleanup, and the addedshortCode+environment.project.externalReffields enable S2 integration without extra queries.enqueueBuildwraps the platform enqueue call in a neverthrowResultAsync, giving InitializeDeployment a clean way to distinguish enqueue failures from other errors.- The S2 helpers (
appendToEventLog,createEventStream,getEventStreamAccessToken) are all gated onenv.S2_ENABLED === "1"and the presence of the shareds2client. When S2 is disabled, they short-circuit with a typeds2_is_disablederror, and callers likecancelDeploymentexplicitly treat those as non-fatal—this aligns with the requirement that S2 remains optional in some environments.- Token caching is sensibly defensive: cache read miss triggers token issuance, and cache write failures are logged but ignored, so log streaming can still proceed.
One small improvement: when
appendToEventLogfails in thecancelDeploymentchain, the log currently only includes the error object. Including basic identifiers likeprojectExternalRefanddeployment.shortCodewould make operational debugging easier, for example:logger.error("Failed to append event to deployment event log", { error, projectExternalRef: deployment.environment.project.externalRef, shortCode: deployment.shortCode, });Also applies to: 126-136, 147-224, 289-305, 307-423, 425-458
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
.changeset/config.json(1 hunks).changeset/proud-birds-change.md(1 hunks)apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/routes/api.v1.artifacts.ts(1 hunks)apps/webapp/app/routes/api.v1.deployments.ts(2 hunks)apps/webapp/app/services/platform.v3.server.ts(1 hunks)apps/webapp/app/v3/services/artifacts.server.ts(1 hunks)apps/webapp/app/v3/services/deployment.server.ts(8 hunks)apps/webapp/app/v3/services/deploymentIndexFailed.server.ts(3 hunks)apps/webapp/app/v3/services/failDeployment.server.ts(2 hunks)apps/webapp/app/v3/services/finalizeDeployment.server.ts(3 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(5 hunks)apps/webapp/app/v3/services/timeoutDeployment.server.ts(4 hunks)apps/webapp/package.json(2 hunks)internal-packages/database/prisma/schema.prisma(1 hunks)packages/cli-v3/package.json(4 hunks)packages/cli-v3/src/apiClient.ts(2 hunks)packages/cli-v3/src/cli/common.ts(1 hunks)packages/cli-v3/src/commands/deploy.ts(8 hunks)packages/cli-v3/src/commands/workers/build.ts(1 hunks)packages/cli-v3/src/deploy/archiveContext.ts(1 hunks)packages/cli-v3/src/deploy/buildImage.ts(1 hunks)packages/cli-v3/src/utilities/windows.ts(3 hunks)packages/core/src/v3/schemas/api.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/cli-v3/src/commands/workers/build.ts
- packages/cli-v3/src/deploy/archiveContext.ts
- apps/webapp/app/env.server.ts
- apps/webapp/package.json
- .changeset/config.json
- apps/webapp/app/services/platform.v3.server.ts
- internal-packages/database/prisma/schema.prisma
- packages/cli-v3/package.json
- .changeset/proud-birds-change.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
📚 Learning: 2024-10-22T10:50:41.311Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 1424
File: packages/core/src/v3/errors.ts:155-189
Timestamp: 2024-10-22T10:50:41.311Z
Learning: When using `assertExhaustive` in a `switch` statement in TypeScript (e.g., in the `shouldRetryError` function in `packages/core/src/v3/errors.ts`), and it throws an error, it's acceptable not to add a `return` statement afterward, as control flow will not proceed beyond the `assertExhaustive` call.
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-10T09:09:07.399Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2663
File: apps/webapp/app/env.server.ts:1205-1206
Timestamp: 2025-11-10T09:09:07.399Z
Learning: In the trigger.dev webapp, S2_ACCESS_TOKEN and S2_DEPLOYMENT_LOGS_BASIN_NAME environment variables must remain optional until an OSS version of S2 is available, to avoid breaking environments that don't have S2 provisioned.
Applied to files:
apps/webapp/app/v3/services/deployment.server.ts
🧬 Code graph analysis (6)
apps/webapp/app/v3/services/initializeDeployment.server.ts (2)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)packages/core/src/v3/workers/taskExecutor.ts (1)
result(1285-1332)
packages/core/src/v3/schemas/api.ts (1)
packages/core/src/v3/schemas/common.ts (2)
GitMeta(239-253)GitMeta(255-255)
apps/webapp/app/v3/services/failDeployment.server.ts (1)
apps/webapp/app/v3/services/deployment.server.ts (1)
DeploymentService(25-459)
packages/cli-v3/src/commands/deploy.ts (6)
packages/core/src/v3/schemas/common.ts (2)
GitMeta(239-253)GitMeta(255-255)packages/cli-v3/src/deploy/archiveContext.ts (2)
createContextArchive(91-149)getArchiveSize(151-154)packages/cli-v3/src/mcp/logger.ts (1)
log(14-23)packages/cli-v3/src/utilities/cliOutput.ts (5)
chalkError(24-26)isLinksSupported(7-7)cliLink(140-145)chalkGrey(20-22)chalkWarning(28-30)packages/cli-v3/src/utilities/githubActions.ts (1)
setGithubActionsOutputAndEnvVars(3-27)packages/core/src/v3/schemas/api.ts (4)
DeploymentFinalizedEvent(592-598)DeploymentFinalizedEvent(607-607)DeploymentEvent(600-603)DeploymentEvent(605-605)
apps/webapp/app/v3/services/deployment.server.ts (4)
apps/webapp/app/env.server.ts (1)
env(1252-1252)apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts (1)
deployment(182-200)apps/webapp/app/services/platform.v3.server.ts (1)
enqueueBuild(594-617)packages/core/src/v3/schemas/api.ts (2)
DeploymentEvent(600-603)DeploymentEvent(605-605)
packages/cli-v3/src/apiClient.ts (2)
packages/core/src/v3/schemas/api.ts (4)
CreateArtifactRequestBody(417-421)CreateArtifactRequestBody(423-423)CreateArtifactResponseBody(425-430)CreateArtifactResponseBody(432-432)packages/core/src/v3/apiClient/core.ts (1)
wrapZodFetch(727-767)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (10)
packages/cli-v3/src/deploy/buildImage.ts (1)
459-488: Registry login success message change is safeSwitching the user-facing log to “Successfully logged in to the remote registry” keeps sensitive details out of
onLogwhile the debug logger still includes the concrete host. No behavioral or correctness concerns here.packages/core/src/v3/schemas/api.ts (2)
417-432: Artifact request/response schemas look consistent with presigned-post flowsThe
CreateArtifactRequestBody/CreateArtifactResponseBodyshapes look sane and align with a deployment-context artifact + S3-style presigned POST (key, URL, arbitrary string fields, and an expiry). If you knowcontentLengthmust be non‑negative and integral, you could tighten it to.int().nonnegative(), but that’s purely a validation hardening choice.
434-451: InitializeDeploymentResponseBodyeventStreamextension seems backward compatibleAdding the optional
eventStream.s2.{basin,stream,accessToken}block keeps the existing response fields intact while allowing callers to opt into streaming when present. Clients that don’t know abouteventStreamwill continue to work unchanged, and consumers that do can branch on its presence.apps/webapp/app/v3/services/failDeployment.server.ts (1)
7-8: Appending a finalized “failed” deployment event is a solid additionCreating a
DeploymentServiceinstance and appending a single"finalized"event withresult: "failed"and the error message cleanly ties the fail path into the new deployment event log. Using.orTeeto log but not throw keeps failure to write the event from breaking the primary failure flow, which feels like the right trade-off here.Also applies to: 54-67
apps/webapp/app/v3/services/timeoutDeployment.server.ts (1)
16-21: Timed‑out update + event logging flow looks consistentIncluding
environment.projecton the initial fetch and then logging a"finalized""timed_out"event viaDeploymentServicebefore enqueueing alerts is coherent with the other deployment flows; the best‑effort.orTeelogging keeps the main timeout path resilient. No further changes needed here.Also applies to: 37-61
apps/webapp/app/routes/api.v1.deployments.ts (1)
40-52: Passing througheventStreamaligns with the new response schemaDestructuring
eventStreamfromInitializeDeploymentService.calland including it in theInitializeDeploymentResponseBodykeeps this route aligned with the updated v3 API; the rest of the payload is unchanged and remains backward compatible for consumers that ignore the new field.apps/webapp/app/v3/services/deploymentIndexFailed.server.ts (1)
4-12: Failure finalization + event logging is wired correctlyTyping
FINAL_DEPLOYMENT_STATUSESwithWorkerDeploymentStatusand fetchingenvironment.projectup front, then logging a"finalized""failed"event viaDeploymentServicebefore enqueueing alerts, is consistent with the other deployment flows and keeps failure handling centralized. Looks good.Also applies to: 35-42, 77-90
apps/webapp/app/v3/services/finalizeDeployment.server.ts (1)
1-3: Finalize flow’s event logging is consistent with other deployment outcomesUsing type‑only imports and delegating the
"finalized""succeeded"event toDeploymentService.appendToEventLog, with best‑effort.orTeeerror logging, keeps the finalize path aligned with the failure/timeout flows while not impacting promotion, engine scheduling, or alerts if logging fails. Looks good.Also applies to: 12-12, 81-93
packages/cli-v3/src/apiClient.ts (1)
3-4: Artifact creation API wiring matches existing client patternsThe
createArtifacthelper reuseswrapZodFetchandgetHeaders()consistently with the rest ofCliApiClient, and the access-token guard mirrors other authenticated methods. Looks good to me.Also applies to: 365-375
packages/cli-v3/src/commands/deploy.ts (1)
1019-1242: Native build log streaming and event parsing are robust and backward-compatibleThe S2-based event streaming loop and
DeploymentEventFromStringhelper look well thought out:
- Errors creating the read session are handled gracefully: user is told to inspect the dashboard, and the command exits successfully instead of hanging.
- The
for awaitloop:
- Skips malformed JSON with a debug log only.
- Treats only known
typevalues (logandfinalized), with adefaultbranch that asserts exhaustiveness and logs unknown types without breaking.finalDeploymentEventis required before proceeding; all the fallback branches (no events, only logs, or mid-stream disconnect) surface clear messages and throwOutroCommandErrorso the outer command can render a single coherent outro.- The result
switchcleanly distinguishessucceeded,failed,timed_out, andcanceled, and the success/default cases now usereturn process.exit(0);, which avoids the previous fallthrough issue while keeping the explicit exit codes.Overall this gives users a good streaming UX while degrading nicely when the stream or event shape isn’t as expected.
Also applies to: 1262-1272
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/cli-v3/src/commands/deploy.ts (3)
978-978: Consider using a named constant for the placeholder contentHash.The magic string
"-"is used as a placeholder for native builds. While functional, extracting this to a named constant (e.g.,NATIVE_BUILD_CONTENT_HASH_PLACEHOLDER) would improve code clarity.
1031-1072: Consider consistent process exit patterns.Lines 1038 and 1071 use
process.exit(0)without an explicitreturn, while the switch statement at lines 1190 and 1258 usesreturn process.exit(0). For consistency with the rest of the file, consider addingreturnbeforeprocess.exit(0)calls.
889-1261: Consider extracting logical sections into helper functions.The
handleNativeBuildServerDeployfunction spans 372 lines and handles multiple concerns: archiving, uploading, event streaming, and result processing. While the current implementation is correct and readable, extracting sections like artifact upload (lines 904-971), event streaming (lines 1031-1133), and result handling (lines 1166-1260) into focused helper functions would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlreferences/hello-world/trigger.config.tsis excluded by!references/**
📒 Files selected for processing (5)
packages/cli-v3/package.json(3 hunks)packages/cli-v3/src/build/buildWorker.ts(3 hunks)packages/cli-v3/src/build/extensions.ts(4 hunks)packages/cli-v3/src/commands/deploy.ts(17 hunks)packages/cli-v3/src/utilities/windows.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-v3/src/utilities/windows.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
packages/cli-v3/package.json
📚 Learning: 2024-10-22T10:50:41.311Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 1424
File: packages/core/src/v3/errors.ts:155-189
Timestamp: 2024-10-22T10:50:41.311Z
Learning: When using `assertExhaustive` in a `switch` statement in TypeScript (e.g., in the `shouldRetryError` function in `packages/core/src/v3/errors.ts`), and it throws an error, it's acceptable not to add a `return` statement afterward, as control flow will not proceed beyond the `assertExhaustive` call.
Applied to files:
packages/cli-v3/src/commands/deploy.ts
🧬 Code graph analysis (2)
packages/cli-v3/src/build/buildWorker.ts (2)
packages/cli-v3/src/build/extensions.ts (1)
createBuildContext(57-119)packages/cli-v3/src/utilities/windows.ts (1)
spinner(115-116)
packages/cli-v3/src/build/extensions.ts (2)
packages/core/src/v3/build/extensions.ts (2)
BuildLogger(29-35)BuildExtension(14-22)packages/cli-v3/src/utilities/windows.ts (1)
spinner(115-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
packages/cli-v3/package.json (1)
98-98: Based on verification results, all three new dependencies are compatible with the Node.js engine requirement (>=18.20.0) and have no known security vulnerabilities:
- @s2-dev/[email protected]: Legitimate package from the s2-streamstore project (actively maintained), no engine requirement specified, not deprecated.
- [email protected]: Requires Node >= 4, well-established package, no reported vulnerabilities.
- [email protected]: Requires Node >= 18, latest stable version (v7.x branch). Historical vulnerabilities affected older v4.x, v5.x, and v6.x branches; this version is unaffected.
All dependencies are compatible and secure for production use.
packages/cli-v3/src/build/buildWorker.ts (1)
53-67: Verify log level handling in plain mode.In plain mode,
debug,log, andwarnall route toconsole.log, which eliminates the distinction between log levels. This means debug output will always appear (potentially verbose) and warnings won't stand out from regular logs.Consider whether this is the intended behavior:
- Should warnings use
console.warnto maintain distinction?- Should debug logging be conditionally enabled based on a debug flag?
If the current approach is intentional for plain mode simplicity, no changes are needed.
packages/cli-v3/src/build/extensions.ts (1)
57-76: LGTM: Clean logger injection pattern.The optional logger parameter with fallback to default logging is well-designed. This enables plain mode logging (used in buildWorker) while preserving existing behavior when no custom logger is provided.
packages/cli-v3/src/commands/deploy.ts (9)
81-83: LGTM!The new CLI options are properly typed with sensible defaults. The
detachoption correctly impliesnativeBuildServerat line 186.
312-322: LGTM!The native build server path cleanly separates the new deployment workflow from the existing path, with an early return to prevent fallthrough.
1026-1029: LGTM!The detach mode correctly exits early after initialization, allowing users to queue deployments without waiting.
1074-1133: LGTM!The event streaming loop is well-structured with proper error handling, graceful degradation for unknown events, and correct use of TypeScript's exhaustiveness checking with
satisfies never. The abort controller correctly terminates the stream when the finalized event arrives.
1135-1164: LGTM!The edge case handling correctly addresses scenarios where the stream terminates unexpectedly, providing clear error messages and deployment links for debugging. This defensive coding improves reliability.
1166-1260: LGTM!The switch statement correctly handles all deployment result cases with proper control flow. The explicit
returnstatements afterprocess.exit(0)address the previous linter concern, and the default case helpfully documents forward compatibility for future enum values.
1281-1291: LGTM!The
DeploymentEventFromStringtransformer correctly implements the Zod pattern for parsing and validating JSON strings, with proper error handling viactx.addIssueandz.NEVER.
205-207: LGTM!The plain mode integration is consistent throughout the deployment flow, correctly suppressing interactive elements (intro, spinners, formatted links) and using direct console output when
--plainis specified.Also applies to: 222-222, 329-329, 345-345, 411-411, 453-453, 460-470, 495-498, 592-602, 612-615, 639-672
6-8: Verify API patterns for S2 library version ^0.17.6.I was unable to locate comprehensive documentation for @s2-dev/streamstore version 0.17.6 specifically. The authenticated S2 client is initialized with accessToken, which matches the code at line 1046. However, the usage pattern in the code (
s2.basin().stream().readSession()) does not appear in the available documentation for the latest TypeScript SDK versions. The methodsbasin(),stream(), andreadSession()were not found in the documented REST API examples.This could indicate either:
- Version 0.17.6 exposes lower-level gRPC APIs directly, which differ from the documented REST API surface
- The API has changed significantly between versions
- These are internal/undocumented methods
Please verify that the S2 API usage at lines 1046-1073 aligns with the actual type definitions and available methods in version 0.17.6 of @s2-dev/streamstore.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/cli-v3/src/commands/deploy.ts (1)
1173-1291: This issue was previously flagged but appears unresolved.The
"succeeded"case (line 1203) anddefaultcase (line 1289) still callprocess.exit(0)without an explicitreturnstatement, triggering Biome'snoFallthroughSwitchClauselint error. The previous review comment included fixes, but they don't appear to be applied in the current code.Add explicit
return;statements:outro( `Version ${deployment.version} was deployed ${ isLinksSupported ? `| ${cliLink("Test tasks", rawTestLink)} | ${cliLink( "View deployment", rawDeploymentLink )}` : "" }` ); - return process.exit(0); + process.exit(0); + return; }outro( `Version ${deployment.version} ${ isLinksSupported ? `| ${cliLink("Test tasks", rawTestLink)} | ${cliLink( "View deployment", rawDeploymentLink )}` : "" }` ); - return process.exit(0); + process.exit(0); + return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli-v3/src/commands/deploy.ts(18 hunks)packages/cli-v3/src/deploy/buildImage.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-v3/src/deploy/buildImage.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2685
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.settings/route.tsx:1234-1257
Timestamp: 2025-11-14T19:24:39.536Z
Learning: In the trigger.dev project, version validation for the `useNativeBuildServer` setting cannot be performed at the settings form level because the SDK version is only known at build/deployment time, not when saving project settings.
📚 Learning: 2024-10-22T10:50:41.311Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 1424
File: packages/core/src/v3/errors.ts:155-189
Timestamp: 2024-10-22T10:50:41.311Z
Learning: When using `assertExhaustive` in a `switch` statement in TypeScript (e.g., in the `shouldRetryError` function in `packages/core/src/v3/errors.ts`), and it throws an error, it's acceptable not to add a `return` statement afterward, as control flow will not proceed beyond the `assertExhaustive` call.
Applied to files:
packages/cli-v3/src/commands/deploy.ts
🧬 Code graph analysis (1)
packages/cli-v3/src/commands/deploy.ts (7)
packages/cli-v3/src/utilities/tempDirectories.ts (1)
getTmpDir(21-49)packages/cli-v3/src/utilities/windows.ts (1)
spinner(115-116)packages/cli-v3/src/mcp/logger.ts (1)
log(14-23)packages/cli-v3/src/utilities/cliOutput.ts (4)
isLinksSupported(7-7)chalkError(24-26)cliLink(140-145)chalkGrey(20-22)packages/cli-v3/src/apiClient.ts (1)
CliApiClient(59-810)packages/cli-v3/src/deploy/archiveContext.ts (1)
createContextArchive(91-149)packages/core/src/v3/schemas/api.ts (4)
DeploymentFinalizedEvent(592-598)DeploymentFinalizedEvent(607-607)DeploymentEvent(600-603)DeploymentEvent(605-605)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
packages/cli-v3/src/commands/deploy.ts (12)
1-60: LGTM: Imports are appropriate for the new native build server functionality.All newly imported modules (S2 for event streaming, artifact management utilities, and chalk styling helpers) are properly utilized in the native build server deployment flow.
81-83: LGTM: Schema additions are well-defined.The new boolean options (
nativeBuildServer,detach,plain) have appropriate default values and are properly integrated with the command configuration.
176-188: LGTM: Command options are properly configured.The
--detachoption correctly implies--native-build-serverusing Commander's.implies()method, ensuring logical consistency. The--plainoption is appropriately hidden from help output.
205-207: LGTM: Plain mode conditionally skips intro.Appropriately suppresses the intro prompt when
plainmode is enabled, which is useful for CI environments and automated workflows.
312-322: LGTM: Clean separation of native build server flow.The early return when
nativeBuildServeris enabled provides clean separation from the standard deployment flow, with all necessary context (apiClient, config, dashboardUrl, userId, gitMeta) passed to the handler.
905-977: LGTM: Archive creation and upload flow is well-structured.The artifact upload sequence properly:
- Creates a temporary archive with automatic cleanup
- Obtains presigned upload credentials from the API
- Constructs FormData with the required fields
- Handles cleanup failures gracefully (lines 966-969) without throwing
Note: The upload error handling at lines 959-963 has been flagged in a previous review comment.
978-1025: LGTM: Native deployment initialization is correctly implemented.Key highlights:
contentHash: "-"is appropriate for native builds where the manifest hasn't been built yet (line 979)isNativeBuild: trueflag properly distinguishes native from standard builds (line 984), consistent with line 378- Path normalization for Windows (line 976) prevents issues with backslash separators
- GitHub Actions outputs are set early, enabling the
--detachmode to exit with useful metadata
1027-1030: LGTM: Detach mode provides clean early exit.When
--detachis specified, the command exits immediately after queuing the deployment, which is useful for CI pipelines that don't need to wait for build completion.
1042-1061: LGTM: S2 event stream setup is correctly configured.The stream configuration properly:
- Uses the access token from the deployment response
- Sets a reasonable 20-minute timeout for build operations (line 1055)
- Includes an AbortController for clean stream termination (lines 1045, 1058)
- Wraps the async operation in
tryCatchfor error handling
1075-1140: LGTM: Event streaming loop is robust and well-structured.Excellent implementation:
- Gracefully handles parse failures by logging and skipping invalid records (lines 1082-1088)
- Uses
satisfies neverin the default case (line 1135) for compile-time exhaustiveness checking- Properly manages spinner state transitions (lines 1094-1098)
- Formats log messages with appropriate severity styling (lines 1110-1118)
- Cleanly terminates the stream when finalized (lines 1131-1132)
1142-1171: LGTM: Comprehensive edge case handling.These conditional checks handle rare but possible scenarios:
- Build never dequeued from the queue (lines 1142-1155)
- Stream terminated prematurely after build started (lines 1157-1171)
Both cases provide clear error messages and appropriate links to the dashboard, demonstrating good defensive programming.
1312-1322: LGTM: Robust JSON parsing schema.The
DeploymentEventFromStringschema correctly:
- Parses JSON strings with proper error handling (lines 1315-1320)
- Pipes the result into the
DeploymentEventdiscriminated union for type safety (line 1322)- Returns
z.NEVERon parse failure to halt validationThis is a solid pattern for validating streamed event data.
description to be added soon