fix(cli-v3): authenticate to Docker Hub to prevent rate limits (#2911) #2992
Conversation
…t build server failures (triggerdotdev#2913)
🦋 Changeset detectedLatest commit: 8b684e1 The changes in this PR will be included in the next version bump. 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 |
WalkthroughThis pull request introduces multiple bug fixes across the CLI and core packages. The ConsoleInterceptor now delegates to original console methods to preserve log chains when other interceptors are present. The deploy command passes an ignoreEngines flag to bypass Node version checks during dependency updates. Docker authentication support is added via DOCKER_USERNAME and DOCKER_PASSWORD environment variables, with login/logout flows around image building. A new ignoreEngines option is added to UpdateCommandOptions, injecting engine-compatibility flags specific to each package manager (npm, pnpm, yarn). URL construction for test links is refactored for consistency, and comprehensive tests validate the engine-ignore flag behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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 |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Docker Hub logout skipped when build fails, leaving credentials logged in
When the Docker build process fails, the code logs out from cloudRegistryHost but does NOT log out from Docker Hub, leaving the session authenticated.
Click to expand
Problem
At lines 642-652, when buildProcess.exitCode !== 0, the code only handles cloudRegistryHost logout:
if (buildProcess.exitCode !== 0) {
if (cloudRegistryHost) {
await x("docker", ["logout", cloudRegistryHost]);
}
return { ok: false, ... }; // Returns without Docker Hub logout!
}Compare to the success path at lines 698-701 which properly logs out from Docker Hub:
if (loggedInToDockerHub) {
logger.debug("Logging out from Docker Hub");
await x("docker", ["logout"]);
}Impact
If the build fails after successfully logging into Docker Hub, the Docker credentials remain cached on the system. This is a minor security/resource cleanup issue.
(Refers to lines 642-652)
Recommendation: Add Docker Hub logout before the early return: if (loggedInToDockerHub) { await x("docker", ["logout"]); } inside the if (buildProcess.exitCode !== 0) block.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const UpdateCommandOptions = CommonCommandOptions.pick({ | ||
| logLevel: true, | ||
| skipTelemetry: true, | ||
| ignoreEngines: true, |
There was a problem hiding this comment.
🔴 Missing ignoreEngines field in CommonCommandOptions causes feature to never work
The ignoreEngines feature added in this PR will never work because ignoreEngines is not defined in CommonCommandOptions schema.
Click to expand
Problem
In update.ts, the code tries to pick ignoreEngines from CommonCommandOptions:
export const UpdateCommandOptions = CommonCommandOptions.pick({
logLevel: true,
skipTelemetry: true,
ignoreEngines: true, // This key doesn't exist!
});However, CommonCommandOptions in common.ts:12-17 only defines:
export const CommonCommandOptions = z.object({
apiUrl: z.string().optional(),
logLevel: z.enum([...]).default("log"),
skipTelemetry: z.boolean().default(false),
profile: z.string().default(...),
});Impact
When using Zod's .pick() with a non-existent key, it silently ignores it. The resulting UpdateCommandOptions schema won't include ignoreEngines, so options.ignoreEngines at line 263 will always be undefined. This means the engine-ignore flags (--no-engine-strict, --config.engine-strict=false, --ignore-engines) will never be passed to package managers, completely defeating the purpose of the feature.
Recommendation: Add ignoreEngines: z.boolean().default(false) to CommonCommandOptions in packages/cli-v3/src/cli/common.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.changeset/fix-docker-hub-rate-limit-2911.md:
- Line 5: The changelog line contains a duplicated word "checking checking" in
the sentence about environment variables; edit the text to remove the duplicate
so it reads "checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment
variables and logging into Docker Hub before building" (search for the string
"checking checking `DOCKER_USERNAME`" to locate and fix it).
In `@packages/cli-v3/src/commands/update.test.ts`:
- Around line 1-112: The tests for updateTriggerPackages currently violate the
repo policy by using many vi.mock calls (see vi.mock of "nypm", "pkg-types",
"node:fs/promises", "@clack/prompts", etc.) — replace these unit-style mocks
with integration-style setup: remove all vi.mock and vi.mocked usage and instead
spin up real test fixtures (or testcontainers) that provide a real package.json
on disk, a real package manager environment or a minimal harness to simulate
npm/pnpm/yarn behavior, and real filesystem operations; keep the same assertions
against updateTriggerPackages(...) but exercise the function against real files
and processes (use helper utilities for creating temp project dirs, writing
package.json, and controlling ENV to select package manager) so tests comply
with the no-mocks requirement while still verifying args passed to
nypm.installDependencies indirectly via the integration harness.
In `@packages/cli-v3/src/commands/update.ts`:
- Around line 261-282: The engine flag handling in the update flow is wrong:
when options.ignoreEngines is set and packageManager.name is checked inside the
switch (see installArgs, packageManager.name), replace the npm and yarn
flags—use "--engine-strict=false" for npm instead of "--no-engine-strict", keep
pnpm as "--config.engine-strict=false", and for yarn (modern Berry v2+) do not
push "--ignore-engines"; instead either push "--force" if you want to bypass
enforcement or skip adding a flag and document that modern Yarn cannot be
bypassed; ensure installDependencies call still receives installArgs and that
installSpinner error messaging remains unchanged.
In `@packages/cli-v3/src/deploy/buildImage.ts`:
- Around line 476-508: After a successful Docker Hub login (when
loggedInToDockerHub is set true after using loginProcess), wrap the subsequent
flow that may early-return (registry login, build steps, etc.) in a try/finally
so that a docker logout is always executed in the finally block; call the same
logout logic (the code currently used around lines 698-701) when
loggedInToDockerHub is true, await any async logout command, and emit
options.onLog?.("Logged out of Docker Hub") or similar. Ensure the finally runs
even if you return early from functions that inspect loginProcess.exitCode or
push errors, and keep the loggedInToDockerHub flag and loginProcess handling
(stdin writes and for-await loop) as-is while centralizing the logout to the
finally block.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/fix-console-interceptor-2900.md.changeset/fix-docker-hub-rate-limit-2911.md.changeset/fix-github-install-node-version-2913.mdpackages/cli-v3/src/commands/deploy.tspackages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/core/src/v3/consoleInterceptor.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
packages/cli-v3/src/commands/update.test.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
packages/cli-v3/src/commands/update.test.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use vitest for all tests in the Trigger.dev repository
Files:
packages/cli-v3/src/commands/update.test.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:
packages/cli-v3/src/commands/update.test.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
packages/cli-v3/src/commands/update.test.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.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
Place test files next to source files with naming pattern: source file (e.g.,MyService.ts) →MyService.test.ts
Files:
packages/cli-v3/src/commands/update.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use testcontainers helpers (
redisTest,postgresTest,containerTest) from@internal/testcontainersfor Redis/PostgreSQL testing instead of mocks
Files:
packages/cli-v3/src/commands/update.test.ts
{packages,integrations}/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Add a changeset when modifying any public package in
packages/*orintegrations/*usingpnpm run changeset:add
Files:
packages/cli-v3/src/commands/update.test.tspackages/core/src/v3/consoleInterceptor.tspackages/cli-v3/src/commands/update.tspackages/cli-v3/src/deploy/buildImage.tspackages/cli-v3/src/commands/deploy.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:
packages/core/src/v3/consoleInterceptor.ts
🧠 Learnings (14)
📚 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:
packages/cli-v3/src/commands/update.test.ts
📚 Learning: 2025-11-27T16:26:44.496Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/executing-commands.mdc:0-0
Timestamp: 2025-11-27T16:26:44.496Z
Learning: For running tests, navigate into the package directory and run `pnpm run test --run` to enable single-file test execution (e.g., `pnpm run test ./src/engine/tests/ttl.test.ts --run`)
Applied to files:
packages/cli-v3/src/commands/update.test.ts
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to {packages,integrations}/**/* : Add a changeset when modifying any public package in `packages/*` or `integrations/*` using `pnpm run changeset:add`
Applied to files:
packages/cli-v3/src/commands/update.test.ts.changeset/fix-github-install-node-version-2913.md
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Use build extensions in trigger.config.ts (additionalFiles, additionalPackages, aptGet, prismaExtension, etc.) to customize the build
Applied to files:
packages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/deploy.ts.changeset/fix-github-install-node-version-2913.md
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vitest exclusively for testing and never mock anything - use testcontainers instead
Applied to files:
packages/cli-v3/src/commands/update.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:
packages/cli-v3/src/commands/update.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 packages/trigger-sdk/**/*.{ts,tsx} : In the Trigger.dev SDK (packages/trigger-sdk), prefer isomorphic code like fetch and ReadableStream instead of Node.js-specific code
Applied to files:
packages/cli-v3/src/commands/update.test.tspackages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-08-20T07:41:13.973Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2424
File: apps/supervisor/src/workloadManager/docker.ts:175-178
Timestamp: 2025-08-20T07:41:13.973Z
Learning: In dockerode, the createImage() method accepts auth configuration as the first parameter and options as the second parameter, not as a property within a single options object. The correct signature is: createImage(auth, options, callback).
Applied to files:
packages/cli-v3/src/deploy/buildImage.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/**/*.{ts,tsx} : Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure build process in trigger.config.ts using `build` object with external packages, extensions, and JSX settings
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export tasks with unique IDs within the project to enable proper task discovery and execution
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `trigger.dev/sdk/v3` for all imports in Trigger.dev tasks
Applied to files:
packages/cli-v3/src/commands/deploy.ts.changeset/fix-github-install-node-version-2913.md
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure Trigger.dev project in `trigger.config.ts` using `defineConfig()` with project ref and task directories
Applied to files:
packages/cli-v3/src/commands/deploy.ts
📚 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/fix-github-install-node-version-2913.md
🔇 Additional comments (7)
.changeset/fix-console-interceptor-2900.md (1)
1-5: Changelog entry looks good..changeset/fix-github-install-node-version-2913.md (1)
1-5: Changelog entry is clear and concise.packages/cli-v3/src/deploy/buildImage.ts (1)
587-592: Cache-to/from argument consolidation looks good.packages/cli-v3/src/commands/update.ts (1)
18-22: Good:ignoreEnginesis wired into the options schema.packages/cli-v3/src/commands/deploy.ts (2)
254-256: Good: deployment update path now bypasses engine checks.
492-494: String interpolation cleanup is consistent.Also applies to: 709-712, 728-735, 742-743, 786-788, 1085-1088, 1153-1155, 1200-1203, 1236-1238, 1251-1253, 1277-1284, 1295-1304, 1314-1323, 1333-1342, 1359-1366
packages/core/src/v3/consoleInterceptor.ts (1)
16-63: Guard against nested intercepts overwriting the original console snapshot.
this.originalConsoleis shared across invocations; ifinterceptis called reentrantly or concurrently on the same instance, the snapshot can be overwritten and the outer restore may leave console methods patched. If nesting is possible, consider a stack (or AsyncLocalStorage) and pop infinally.🧠 Suggested fix (support nested intercepts)
- private originalConsole: + private originalConsoleStack: + Array<{ + log: Console["log"]; + info: Console["info"]; + warn: Console["warn"]; + error: Console["error"]; + debug: Console["debug"]; + }> = []; - - this.originalConsole = { + const originalConsole = { log: console.log, info: console.info, warn: console.warn, error: console.error, debug: console.debug, }; + this.originalConsoleStack.push(originalConsole); - if (this.originalConsole) { - console.log = this.originalConsole.log; - console.info = this.originalConsole.info; - console.warn = this.originalConsole.warn; - console.error = this.originalConsole.error; - console.debug = this.originalConsole.debug; - - this.originalConsole = undefined; - } + const originalConsole = this.originalConsoleStack.pop(); + if (originalConsole) { + console.log = originalConsole.log; + console.info = originalConsole.info; + console.warn = originalConsole.warn; + console.error = originalConsole.error; + console.debug = originalConsole.debug; + } - if (this.originalConsole) { + const originalConsole = this.originalConsoleStack.at(-1); + if (originalConsole) { // use originalConsole.* } else { // stdout/stderr fallback }Also applies to: 95-119
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "@trigger.dev/cli-v3": patch | ||
| --- | ||
|
|
||
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (#2911) |
There was a problem hiding this comment.
Fix duplicated word in changelog.
There’s a typo (“checking checking”) in the user-facing release note.
✏️ Proposed fix
-Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`)
+Fix: Native build server failed with Docker Hub rate limits. Added support for checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (#2911) | |
| Fix: Native build server failed with Docker Hub rate limits. Added support for checking `DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging into Docker Hub before building. (`#2911`) |
🤖 Prompt for AI Agents
In @.changeset/fix-docker-hub-rate-limit-2911.md at line 5, The changelog line
contains a duplicated word "checking checking" in the sentence about environment
variables; edit the text to remove the duplicate so it reads "checking
`DOCKER_USERNAME` and `DOCKER_PASSWORD` in environment variables and logging
into Docker Hub before building" (search for the string "checking checking
`DOCKER_USERNAME`" to locate and fix it).
|
|
||
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; | ||
| import { updateTriggerPackages } from "./update.js"; | ||
| import * as nypm from "nypm"; | ||
| import * as pkgTypes from "pkg-types"; | ||
| import * as fs from "node:fs/promises"; | ||
| import * as clack from "@clack/prompts"; | ||
| import path from "node:path"; | ||
|
|
||
| // Mock dependencies | ||
| vi.mock("nypm"); | ||
| vi.mock("pkg-types"); | ||
| vi.mock("node:fs/promises"); | ||
| vi.mock("@clack/prompts"); | ||
| vi.mock("std-env", () => ({ | ||
| hasTTY: true, | ||
| isCI: false, | ||
| })); | ||
| vi.mock("../utilities/initialBanner.js", () => ({ | ||
| updateCheck: vi.fn().mockResolvedValue(undefined), | ||
| printStandloneInitialBanner: vi.fn(), | ||
| })); | ||
| vi.mock("../version.js", () => ({ | ||
| VERSION: "3.0.0", | ||
| })); | ||
| vi.mock("../cli/common.js", () => ({ | ||
| CommonCommandOptions: { pick: () => ({}) }, | ||
| })); | ||
| vi.mock("../utilities/cliOutput.js", () => ({ | ||
| chalkError: vi.fn(), | ||
| prettyError: vi.fn(), | ||
| prettyWarning: vi.fn(), | ||
| })); | ||
| vi.mock("../utilities/fileSystem.js", () => ({ | ||
| removeFile: vi.fn(), | ||
| writeJSONFilePreserveOrder: vi.fn(), | ||
| })); | ||
| vi.mock("../utilities/logger.js", () => ({ | ||
| logger: { | ||
| debug: vi.fn(), | ||
| log: vi.fn(), | ||
| table: vi.fn(), | ||
| }, | ||
| })); | ||
| vi.mock("../utilities/windows.js", () => ({ | ||
| spinner: () => ({ | ||
| start: vi.fn(), | ||
| message: vi.fn(), | ||
| stop: vi.fn(), | ||
| }), | ||
| })); | ||
|
|
||
| describe("updateTriggerPackages", () => { | ||
| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
|
|
||
| // Default mocks | ||
| vi.mocked(fs.writeFile).mockResolvedValue(undefined); | ||
| vi.mocked(fs.rm).mockResolvedValue(undefined); | ||
| vi.mocked(pkgTypes.readPackageJSON).mockResolvedValue({ | ||
| dependencies: { | ||
| "@trigger.dev/sdk": "2.0.0", // Mismatch | ||
| }, | ||
| }); | ||
| vi.mocked(pkgTypes.resolvePackageJSON).mockResolvedValue("/path/to/package.json"); | ||
| vi.mocked(clack.confirm).mockResolvedValue(true); // User confirms update | ||
| vi.mocked(nypm.installDependencies).mockResolvedValue(undefined); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("should pass --no-engine-strict for npm when ignoreEngines is true", async () => { | ||
| vi.mocked(nypm.detectPackageManager).mockResolvedValue({ name: "npm", command: "npm", version: "1.0.0" } as any); | ||
|
|
||
| await updateTriggerPackages(".", { ignoreEngines: true } as any, true, true); | ||
|
|
||
| expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ | ||
| args: ["--no-engine-strict"], | ||
| })); | ||
| }); | ||
|
|
||
| it("should pass --config.engine-strict=false for pnpm when ignoreEngines is true", async () => { | ||
| vi.mocked(nypm.detectPackageManager).mockResolvedValue({ name: "pnpm", command: "pnpm", version: "1.0.0" } as any); | ||
|
|
||
| await updateTriggerPackages(".", { ignoreEngines: true } as any, true, true); | ||
|
|
||
| expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ | ||
| args: ["--config.engine-strict=false"], | ||
| })); | ||
| }); | ||
|
|
||
| it("should pass --ignore-engines for yarn when ignoreEngines is true", async () => { | ||
| vi.mocked(nypm.detectPackageManager).mockResolvedValue({ name: "yarn", command: "yarn", version: "1.0.0" } as any); | ||
|
|
||
| await updateTriggerPackages(".", { ignoreEngines: true } as any, true, true); | ||
|
|
||
| expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ | ||
| args: ["--ignore-engines"], | ||
| })); | ||
| }); | ||
|
|
||
| it("should NOT pass engine flags if ignoreEngines is false (default)", async () => { | ||
| vi.mocked(nypm.detectPackageManager).mockResolvedValue({ name: "npm", command: "npm", version: "1.0.0" } as any); | ||
|
|
||
| await updateTriggerPackages(".", { ignoreEngines: false } as any, true, true); | ||
|
|
||
| expect(nypm.installDependencies).toHaveBeenCalledWith(expect.objectContaining({ | ||
| args: [], | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Mocks in this test conflict with repo testing policy.
This suite relies on extensive vi.mock usage; the repo standard is to avoid mocks and use testcontainers/integration-style helpers instead. Please rework these tests to align with the no-mocks policy.
As per coding guidelines: Use vitest exclusively for testing and never mock anything - use testcontainers instead.
🤖 Prompt for AI Agents
In `@packages/cli-v3/src/commands/update.test.ts` around lines 1 - 112, The tests
for updateTriggerPackages currently violate the repo policy by using many
vi.mock calls (see vi.mock of "nypm", "pkg-types", "node:fs/promises",
"@clack/prompts", etc.) — replace these unit-style mocks with integration-style
setup: remove all vi.mock and vi.mocked usage and instead spin up real test
fixtures (or testcontainers) that provide a real package.json on disk, a real
package manager environment or a minimal harness to simulate npm/pnpm/yarn
behavior, and real filesystem operations; keep the same assertions against
updateTriggerPackages(...) but exercise the function against real files and
processes (use helper utilities for creating temp project dirs, writing
package.json, and controlling ENV to select package manager) so tests comply
with the no-mocks requirement while still verifying args passed to
nypm.installDependencies indirectly via the integration harness.
| const installArgs: string[] = []; | ||
|
|
||
| if (options.ignoreEngines && packageManager) { | ||
| switch (packageManager.name) { | ||
| case "npm": | ||
| installArgs.push("--no-engine-strict"); | ||
| break; | ||
| case "pnpm": | ||
| installArgs.push("--config.engine-strict=false"); | ||
| break; | ||
| case "yarn": | ||
| installArgs.push("--ignore-engines"); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| await installDependencies({ cwd: projectPath, silent: true, args: installArgs }); | ||
| } catch (error) { | ||
| installSpinner.stop( | ||
| `Failed to install new package versions${ | ||
| packageManager ? ` with ${packageManager.name}` : "" | ||
| `Failed to install new package versions${packageManager ? ` with ${packageManager.name}` : "" | ||
| }` | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
npm --no-engine-strict flag documentation
💡 Result:
--no-engine-strict isn’t documented as its own npm flag. What npm documents is the engine-strict config (boolean) and the generic rule that any config can be set on the command line with --option-name value. (docs.npmjs.com)
So, instead of --no-engine-strict, use the documented form:
npm install --engine-strict=falseAnd to enable it:
npm install --engine-strict
# (equivalent to --engine-strict=true)What it does: when engine-strict is true, npm refuses to install packages whose engines don’t match your current Node.js version; it can be overridden with --force. (docs.npmjs.com)
References
[1] npm Docs – config (CLI flags + engine-strict) (docs.npmjs.com)
Citations:
- 1: https://docs.npmjs.com/cli/v11/using-npm/config?utm_source=openai
- 2: https://docs.npmjs.com/cli/v11/using-npm/config?utm_source=openai
- 3: https://docs.npmjs.com/cli/v11/using-npm/config?utm_source=openai
🌐 Web query:
pnpm --config.engine-strict=false flag documentation
💡 Result:
engine-strict is a pnpm setting (from .npmrc / pnpm settings), not a dedicated pnpm install option. The documented way to pass it inline on the command line is via --config.<key>=<value>:
pnpm install --config.engine-strict=false
# (or pnpm add ... --config.engine-strict=false)This --config. mechanism is documented in pnpm’s CLI docs (“force the unknown option with --config.”). [1]
The setting itself is documented as engine-strict (default false) in pnpm’s Settings / .npmrc docs, and you can also set it persistently via:
# .npmrc
engine-strict=false
``` [2]
**Sources:** [1] [2]
---
🌐 Web query:
`yarn --ignore-engines flag v1 v2 Berry documentation`
💡 Result:
- **Yarn v1 (Classic)** supports `--ignore-engines` (skips the `engines` compatibility check). It’s documented on the Classic `yarn install` page under “`yarn install --ignore-engines`”. [1]
- **Yarn v2+ (Berry / modern Yarn)**: the current `yarn install` docs **do not list** a `--ignore-engines` option. [2] Relatedly, Berry has an open feature request to “return / restore” the old Yarn 1.x-style `engines` check behavior (“Just restore old behavior from 1.x”). [3]
**Sources:**
[1] Yarn Classic `yarn install` docs (includes `--ignore-engines`) ([classic.yarnpkg.com](https://classic.yarnpkg.com/en/docs/cli/install?utm_source=openai))
[2] Yarn (Berry) `yarn install` docs (options list; no `--ignore-engines`) ([yarnpkg.com](https://yarnpkg.com/cli/install?utm_source=openai))
[3] Berry issue: “[Feature] Return ‘engines’ check” (`#1177`) ([github.com](https://github.com/yarnpkg/berry/issues/1177))
Citations:
- 1: https://classic.yarnpkg.com/en/docs/cli/install?utm_source=openai
- 2: https://yarnpkg.com/cli/install?utm_source=openai
- 3: https://github.com/yarnpkg/berry/issues/1177
---
🏁 Script executed:
```shell
# Check if there's any version pinning or nypm configuration in the project
cd packages/cli-v3 && find . -name "package.json" -type f | head -5 | xargs cat | grep -E "(nypm|npm|pnpm|yarn)" | head -20Repository: triggerdotdev/trigger.dev
Length of output: 661
🏁 Script executed:
# Look for nypm usage and version info
rg "nypm" packages/cli-v3/src/commands/update.ts -B 5 -A 5Repository: triggerdotdev/trigger.dev
Length of output: 592
🏁 Script executed:
# Check the full context of how packageManager is obtained
rg "packageManager" packages/cli-v3/src/commands/update.ts -B 10 -A 10 | head -50Repository: triggerdotdev/trigger.dev
Length of output: 1481
Fix npm and Yarn engine-strict flags; they are incorrect for the current versions.
- npm:
--no-engine-strictis not a valid flag. Use--engine-strict=falseinstead. - pnpm:
--config.engine-strict=falseis correct. - yarn:
--ignore-enginesis not supported in Yarn Berry (v2+). The project uses Yarn 4.2.2, which does not have this option. Consider either using--forceas an alternative or noting that engine enforcement cannot be bypassed in modern Yarn.
🤖 Prompt for AI Agents
In `@packages/cli-v3/src/commands/update.ts` around lines 261 - 282, The engine
flag handling in the update flow is wrong: when options.ignoreEngines is set and
packageManager.name is checked inside the switch (see installArgs,
packageManager.name), replace the npm and yarn flags—use "--engine-strict=false"
for npm instead of "--no-engine-strict", keep pnpm as
"--config.engine-strict=false", and for yarn (modern Berry v2+) do not push
"--ignore-engines"; instead either push "--force" if you want to bypass
enforcement or skip adding a flag and document that modern Yarn cannot be
bypassed; ensure installDependencies call still receives installArgs and that
installSpinner error messaging remains unchanged.
| // Authenticate to Docker Hub if credentials are provided (fixes rate limit issues) | ||
| let loggedInToDockerHub = false; | ||
| if (process.env.DOCKER_USERNAME && process.env.DOCKER_PASSWORD) { | ||
| logger.debug("Logging in to Docker Hub"); | ||
| const loginProcess = x( | ||
| "docker", | ||
| ["login", "--username", process.env.DOCKER_USERNAME, "--password-stdin"], | ||
| { | ||
| nodeOptions: { | ||
| cwd: options.cwd, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| loginProcess.process?.stdin?.write(process.env.DOCKER_PASSWORD); | ||
| loginProcess.process?.stdin?.end(); | ||
|
|
||
| for await (const line of loginProcess) { | ||
| errors.push(line); | ||
| logger.debug(line); | ||
| } | ||
|
|
||
| if (loginProcess.exitCode !== 0) { | ||
| return { | ||
| ok: false as const, | ||
| error: `Failed to login to Docker Hub`, | ||
| logs: extractLogs(errors), | ||
| }; | ||
| } | ||
|
|
||
| loggedInToDockerHub = true; | ||
| options.onLog?.("Successfully logged in to Docker Hub"); | ||
| } |
There was a problem hiding this comment.
Ensure Docker Hub credentials are logged out on all exit paths.
Logout currently only happens after a successful build; any early return after a successful login (e.g., registry login failure or build failure) leaves Docker Hub credentials on the host. Consider wrapping the post-login flow in a try/finally (or otherwise centralize logout) so it always runs.
🔒 Suggested fix (ensure logout runs even on failures)
- let loggedInToDockerHub = false;
+ let loggedInToDockerHub = false;
+ const logoutDockerHub = async () => {
+ if (loggedInToDockerHub) {
+ logger.debug("Logging out from Docker Hub");
+ await x("docker", ["logout"]);
+ }
+ };
- if (process.env.DOCKER_USERNAME && process.env.DOCKER_PASSWORD) {
+ if (process.env.DOCKER_USERNAME && process.env.DOCKER_PASSWORD) {
...
- loggedInToDockerHub = true;
- options.onLog?.("Successfully logged in to Docker Hub");
- }
+ loggedInToDockerHub = true;
+ options.onLog?.("Successfully logged in to Docker Hub");
+ }
+ try {
+ // existing registry-login + build flow
+ } finally {
+ await logoutDockerHub();
+ }
-
- if (loggedInToDockerHub) {
- logger.debug("Logging out from Docker Hub");
- await x("docker", ["logout"]);
- }Also applies to: 698-701
🤖 Prompt for AI Agents
In `@packages/cli-v3/src/deploy/buildImage.ts` around lines 476 - 508, After a
successful Docker Hub login (when loggedInToDockerHub is set true after using
loginProcess), wrap the subsequent flow that may early-return (registry login,
build steps, etc.) in a try/finally so that a docker logout is always executed
in the finally block; call the same logout logic (the code currently used around
lines 698-701) when loggedInToDockerHub is true, await any async logout command,
and emit options.onLog?.("Logged out of Docker Hub") or similar. Ensure the
finally runs even if you return early from functions that inspect
loginProcess.exitCode or push errors, and keep the loggedInToDockerHub flag and
loginProcess handling (stdin writes and for-await loop) as-is while centralizing
the logout to the finally block.
1 participant
Fixes #2911. Adds logic to \packages/cli-v3/src/deploy/buildImage.ts\ to authenticate with Docker Hub using \DOCKER_USERNAME\ and \DOCKER_PASSWORD\ env vars if present, avoiding 429 rate limit errors for native build server.