feat(cli): warn on createRequire packages missing from deployed images - #3
Conversation
Source PR: triggerdotdev#4851 Source head: b1a98d5
|
| installedPackagesForTarget(target) { | ||
| if (target !== "deploy") { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Shipwright · HIGH
The 'installedPackagesForTarget' hook is documented as diagnostics-only and ignored by the bundler, but the implementation in 'additionalPackages' silently swallows parse errors wi
Impact: The 'installedPackagesForTarget' hook is documented as diagnostics-only and ignored by the bundler, but the implementation in 'additionalPackages' silently swallows parse errors with 'catch { continue; }'. A typo in a package specifier (e.g. 'npm:@scope/pkg@1.0.0' malformed) will silently drop the package from diagnostics, producing a false warning with no indication of why. The user has no way to know their extensi…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| const usages: CreateRequireUsage[] = []; | ||
|
|
||
| for (const entry of scanned) { |
There was a problem hiding this comment.
Shipwright · HIGH
The 'extensionInstalledPackageMatchers' function marks the result incomplete when an 'additionalPackages' extension predates the hook, but this only suppresses dev-mode warnings.
Impact: The 'extensionInstalledPackageMatchers' function marks the result incomplete when an 'additionalPackages' extension predates the hook, but this only suppresses dev-mode warnings. In deploy mode, the warning still fires even though the extension may actually install the package. The comment acknowledges this is intentional, but a user upgrading an old config will see deploy warnings they cannot easily diagnose becaus…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| * `@babel/parser`, so comments, strings, templates, regex literals and JSX | ||
| * can never confuse the scan; a file that fails to parse is skipped | ||
| * (diagnostics must never fail a build). Binding tracking is name-based, | ||
| * module-level, and per-file: computed specifiers, shadowed names, and a |
There was a problem hiding this comment.
Shipwright · HIGH
The createRequire scanner silently skips files that fail to parse (parseWithFallbacks returns undefined and scanSourceForCreateRequire returns []).
Impact: The createRequire scanner silently skips files that fail to parse (parseWithFallbacks returns undefined and scanSourceForCreateRequire returns []). This means a syntax error or unsupported syntax in a user's source file will suppress the warning entirely, and the user will only discover the missing package at runtime in production. The feature's core promise is to warn before deploy, but parse failures create a sile…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| private _usages: CreateRequireUsage[] = []; | ||
| private _cache = new Map<string, CollectorCacheEntry>(); | ||
| private _plugin: esbuild.Plugin | undefined; | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The collector cache is keyed only by mtime and size.
Impact: The collector cache is keyed only by mtime and size. A file modified within the same mtime granularity and with the same byte size will return stale specifiers, causing either missed warnings or warnings for code that no longer exists. This is a classic cache invalidation bug for dev rebuilds.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| let incomplete = false; | ||
|
|
||
| for (const buildExtension of config.build?.extensions ?? []) { | ||
| if (buildExtension.name === "externals") { |
There was a problem hiding this comment.
Shipwright · HIGH
The INSTALL_COMMAND_REGEX matches 'npm install' anywhere in a command string, including inside shell comments, heredocs, or quoted strings.
Impact: The INSTALL_COMMAND_REGEX matches 'npm install' anywhere in a command string, including inside shell comments, heredocs, or quoted strings. A Dockerfile command like 'RUN echo "npm install fake-pkg"' would be parsed as installing fake-pkg, suppressing a legitimate warning. Conversely, complex shell constructs like 'npm install pkg1 && npm install pkg2' are handled, but 'npm install pkg1; npm install pkg2' is not, ca…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| let incomplete = false; | ||
|
|
||
| for (const buildExtension of config.build?.extensions ?? []) { | ||
| if (buildExtension.name === "externals") { |
There was a problem hiding this comment.
Shipwright · HIGH
The 'packagesInstalledByCommands' function parses build-layer commands and treats any token matching an install command as a trusted package name.
Impact: The 'packagesInstalledByCommands' function parses build-layer commands and treats any token matching an install command as a trusted package name. An attacker who can influence the build commands (e.g. via a malicious extension or a compromised config) could inject a package name that suppresses warnings for a genuinely missing dependency, masking a runtime failure or a supply-chain gap. The regex has no anchoring t…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| // Create the regex pattern | ||
| const pattern = `^${escapedPkg}(?:/[^'"]*)?$`; | ||
| export function makeExternalRegexp(packageName: string): RegExp { |
There was a problem hiding this comment.
Shipwright · HIGH
The 'makeExternalRegexp' function is now exported and used to build regexes from extension-declared package names.
Impact: The 'makeExternalRegexp' function is now exported and used to build regexes from extension-declared package names. If an extension returns a package name containing regex metacharacters that are not properly escaped (e.g. a name with '[' or '('), the resulting regex could match unintended packages or fail to match the intended one. The escapeRegExp function is present, but the exported surface increases the risk of…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
|
|
||
| type CollectorCacheEntry = { | ||
| mtimeMs: number; | ||
| size: number; |
There was a problem hiding this comment.
Shipwright · HIGH
The 'CreateRequireCollector' reads and parses every bundle input file outside node_modules, including files that may be generated or user-controlled.
Impact: The 'CreateRequireCollector' reads and parses every bundle input file outside node_modules, including files that may be generated or user-controlled. The parser is configured with 'errorRecovery: true' and multiple fallback plugin sets, but there is no size limit on files read. A malicious or accidental large file in the project could cause memory pressure during the build, especially with FILE_READ_CONCURRENCY=16 r…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| @@ -0,0 +1,729 @@ | |||
| import { parse, ParserPlugin } from "@babel/parser"; | |||
There was a problem hiding this comment.
Shipwright · MEDIUM
The 'isCreateRequireCall' helper only recognizes 'createRequire' calls when the callee is an Identifier alias or a MemberExpression on a namespace import.
Impact: The 'isCreateRequireCall' helper only recognizes 'createRequire' calls when the callee is an Identifier alias or a MemberExpression on a namespace import. It does not recognize the common pattern 'const { createRequire } = require('module')' because 'collectVariableDeclarator' only records ObjectPattern properties when the initializer is a module builtin load, and 'isModuleBuiltinLoad' only accepts 'require('module'…
Suggested fix: Fix the review finding before release.
Summary
The
trigger.dev deployandtrigger.dev devcommands now warn (with the suggested fix) when your code loads a package throughcreateRequire()that won't be available in the deployed image. Previously it would fail at runtime in production to load the package. Deploys also now show bundler warnings for your code instead of discarding them.A package loaded with
createRequire(import.meta.url)("pkg")is invisible to esbuild: the call is never resolved, so the package is neither bundled nor collected as an external to install in the deployed image. The deploy succeeds with zero diagnostics and the task fails at runtime with a module-not-found error, which can surface as something far more confusing when a library maps errors coarsely (a database driver loaded this way can look exactly like a connection failure). It also works fine intrigger devbecause the localnode_modulesexists, making the production-only failure extra misleading.Both
deployanddevbuilds now warn about this, pointing at the exact file and line, with a note showing the exact config that fixes it:In
devthe message instead explains that the code works locally but deploys of it will fail, so the problem is caught while the code is being written rather than after a deploy.How it works
An esbuild plugin scans the bundle's input files outside
node_modulesfor string-literal specifiers passed tocreateRequire-created require functions:createRequire(...)("pkg"),const req = createRequire(...); req("pkg"),req.resolve("pkg"), aliased imports, namespace access, CJS destructuring, and dynamicimport("node:module")bindings. Sources are parsed with@babel/parser(already in the dependency tree), so comments, strings, templates, regex literals and JSX can't confuse the scan; a file that fails to parse is skipped. Relative paths and node builtins never warn.A usage only warns when the package will actually be missing from the image. On deploys the resolved manifest externals are the source of truth (extension-installed layers are already merged in when the warning runs);
build.externalalone deliberately does not suppress, because marking a package external installs nothing when nothing statically imports it. In dev, which predicts a future deploy, suppression additionally trusts what extensions declare they install, and stays silent entirely when that can't be determined (an extension hook throws, or an older@trigger.dev/build's additionalPackages predates the declaration hook), so dev never makes a false "deploys will fail" claim.additionalPackagesdeclares its packages via a new diagnostics-onlyBuildExtensionfield,installedPackagesForTarget, which the bundler ignores: bundling output is unchanged for existing projects.Detection is name-based, module-level, and deliberately per-file: computed specifiers, shadowed names, and require helpers imported from other files are not followed (those degrade to today's behavior, an unwarned runtime failure), and scanning is scoped to user code because bundled libraries legitimately use optional-require patterns that would drown real findings in noise. Packages named in build-layer install commands (
RUN npm install ...) are suppressed individually.Deploys also now surface esbuild's own bundle warnings for user files (for example
require()with a non-literal argument), which were previously discarded on the deploy path;trigger devalready showed them.Verification
Beyond the unit suite (54 tests, including real esbuild builds through the collector plugin), verified end to end against the hello-world reference project with the CLI linked to this branch:
trigger dev: a task loadingmssqlviacreateRequireproduced the dev-phrased warning with the exact file:line code frame and the fix note during "Building local worker", and the local worker started normally. The project's real extensions (lightpanda,syncEnvVars, a custom inline extension) did not suppress it, and none of the project's other task files produced spurious warnings.trigger deploy --dry-run, three passes: with the createRequire task it printed the deploy-phrased warning and still completed; after addingadditionalPackages({ packages: ["mssql"] })the warning disappeared and the build was clean; with the task removed and the config reverted, a pristine build produced zero warnings.Source merge-base:
1d55693c0fc76279e7e8275fe41f959b65d5ea99Source head:
b1a98d535aeefe0aefb2fd7a323702e399bf7c26