fix: honour the lockfile when building nodejs dependencies in source

[bnusunny]

Issue #, if available: reported in aws/aws-sam-cli#6567

no linked issue: aws/aws-sam-cli#6567 asks for more than this fix, so this PR must not close it

Description of changes

Building nodejs dependencies in source ignored the project's lockfile, so a build could upgrade dependencies in the developer's own source tree.

The install step for --build-in-source runs npm update --no-audit --no-save --omit=dev --no-package-lock --install-links. --no-package-lock tells npm not to read the lockfile, so every build re-resolves the ranges in package.json. On an npm workspaces project whose root package-lock.json pins lodash 4.17.20 while the manifest allows ^4.17.20, one sam build --build-in-source left 4.18.1 installed in the source tree.

project	before	after
has a lockfile	npm update --no-audit --no-save --omit=dev --no-package-lock --install-links	npm install -q --no-audit --no-save --omit=dev --install-links
has no lockfile	npm update ...	unchanged

Which directory holds the lockfile npm will use is npm's own decision, so NodejsNpmWorkflow.get_lockfile_path() asks it: npm prefix, run in the directory the install will run in, reports the monorepo root for a workspace package — where npm workspaces keep the single lockfile — and the package itself for any other nested package, whose ancestors' lockfiles npm ignores. The answer is therefore the same one the install acts on, and it cannot wander outside the project. Identical on npm 8.19.4, 9.9.4, 10.9.9 and 11.19.0. NodejsNpmInstallAction takes back an install_links argument for this path.

A project whose root has no lockfile keeps npm update, which is what prunes dependencies removed from the manifest since the previous build (#579). A lockfile-respecting npm install prunes those as well, even though the lockfile it reads still lists the removed package, so projects that do have a lockfile lose nothing. test_build_in_source_with_removed_dependencies_and_a_lockfile covers that path and test_build_in_source_with_removed_dependencies the lockfile-less one.

npm ci stays gated on a lockfile in the source directory itself and deliberately does not use this lookup. Run from a workspace package it deletes node_modules and reifies from the lockfile, emptying the workspace root's dev dependencies on every npm version — in the same fixture it deleted node_modules/.bin/esbuild, the binary the esbuild workflow resolves through npm root — while the install leaves them in place on npm 11.

One thing a monorepo build in this workflow still does not do is carry the hoisted dependencies into the artifacts directory: npm puts them at the monorepo root, node_modules never appears beside the function, and the link step skips a missing source silently. That predates this change and npm update hoisted to the same place, so _actions_for_linking_source_dependencies_to_artifacts carries a comment naming #933 rather than hiding the gap behind the new workspaces coverage. The esbuild workflow is unaffected — it bundles the dependency into the output file.

The new manifests and lockfiles under tests/integration/workflows/nodejs_npm_esbuild/testdata/workspaces-monorepo/, tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-lockfile/, tests/integration/workflows/nodejs_npm/testdata/npm-deps-with-v1-lockfile/ and tests/integration/workflows/nodejs_npm/testdata/with-local-dependency-and-lockfile/ are test fixture data, not a dependency change to this package.

Description of how you validated changes

Five new integration tests. Four fail on develop with AssertionError: '1.5.0' != '1.3.0' because every fixture's lockfile pins minimal-request-promise 1.3.0 while its manifest allows ^1.3.0 (1.5.0 is published).

test_esbuild_can_build_in_source_in_workspaces_monorepo_with_locked_versions covers the workspaces monorepo, where the lockfile sits at the root rather than beside the function. It requires the produced bundle with node, which fails with MODULE_NOT_FOUND if the hoisted dependency was never resolved, so the artifact is covered and not just the installed tree. It asserts the root's workspace link still resolves to the developer's packages/fn rather than a packed snapshot of it — npm exempts workspaces from --install-links, on every major.

test_build_in_source_with_removed_dependencies_and_a_lockfile covers the plain workflow with a lockfile beside the source: the locked version is installed, and a dependency removed from the manifest on the second build is dropped even though the lockfile still lists it.

test_build_in_source_with_a_version_1_lockfile covers the format npm 6 wrote, which npm 7 and later must migrate before they can reify.

test_build_in_source_with_a_local_dependency_and_a_lockfile covers a file: dependency, which is what --install-links exists for. A lockfile written by a plain npm install records that dependency as "resolved": "../npm-deps", "link": true — the symlinked tree --install-links overrides — and reading a lockfile while --install-links is set is a combination that could not arise before, since every build-in-source install ran with --no-package-lock. The test asserts the dependency lands as a real directory holding the package's files, not a symlink pointing outside the artifacts. Setting install_links=False there makes it fail with AssertionError: True is not false.

test_build_in_source_drops_already_installed_dev_dependencies runs the developer's own npm install in the fixture first, so node_modules really holds the ms devDependency when the build starts, then asserts the build leaves it in neither node_modules nor the artifacts. --omit=dev is what drops it, on every npm version and for npm update equally; dropping that flag fails this test. The pre-build install goes through SubprocessNpm, which resolves npm.cmd on Windows.

Four of them read the lockfile bytes before the build and assert they are unchanged after it, at lockfileVersion 1, 2 and 3. The install runs in the developer's own directory and NodejsNpmLockFileCleanUpAction is skipped there, so anything npm wrote would persist; --no-save is what keeps it out. Dropping that flag rewrites the version 1 fixture as version 3 on the first build, which is the assertion that fails. Two fixtures carry a ms devDependency that --omit=dev keeps out of node_modules, so the dev entries of a lockfile are covered too.

New unit tests cover the lockfile lookup against a stubbed npm prefix: found in the project root, found at the workspaces root, npm-shrinkwrap.json, absent, an ancestor's lockfile ignored for a nested non-workspace package, and npm prefix itself failing. Two more pin the install-directory semantics — an external manifest directory's lockfile is honoured, and a lockfile next to the source directory does not count when npm installs somewhere else — plus one for --install-links on the install action. The existing build-in-source wiring tests assert the install action and pin the no-lockfile case to npm update.

• make pr equivalent: pytest --cov aws_lambda_builders --cov-fail-under 94 tests/unit tests/functional — 958 passed, coverage 94.67%. ruff check aws_lambda_builders and black --check clean.
• pytest tests/integration/workflows/nodejs_npm tests/integration/workflows/nodejs_npm_esbuild — 290 passed on npm 11.19.0.
• Because the pruning, the lockfile writes and the npm prefix behaviour above are all npm-version dependent, the lockfile subset was re-run against each npm major CI builds — 8.19.4, 9.9.4, 10.9.9, 11.19.0 — 20 passed on every one, and removing --no-save fails the version 1 test on every one. Standalone probes on all four confirm that npm install --no-audit --no-save --omit=dev --install-links prunes a dependency removed from the manifest, that it overrides a lockfile's "link": true entry with a real directory, that npm prefix reports the monorepo root inside a workspace package and the package itself inside a plain nested one, and that npm installing in that nested package ignores the root lockfile (4.17.20 pinned, 4.18.1 installed) — which is why an ancestor lockfile must not select the install strategy. Linux only locally; the Windows lanes are untested outside CI.
• End to end with SAM CLI 1.166.2 on a two-function npm workspaces project: the build runs NpmInstall instead of NpmUpdate, root lodash stays at the locked 4.17.20 across repeat builds (4.18.1 before), the root esbuild devDependency survives, and the bundle still inlines the local workspace package.

Checklist

• Review the generative AI contribution guidelines

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..e0d4fe9
Files: 10
Comments: 2

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..ce5004a
Files: 10
Comments: 1

[aws-sam-tooling-bot]

[BUG] The upward walk never stops at the npm project root, so it can return a lockfile that npm will not read — the docstring's claim that "the result is a directory npm would itself treat as the project root" is not actually enforced.

The loop only stops walking when it finds a directory that has both a manifest and a lockfile. A directory that has its own package.json but no lockfile is skipped over, even though npm treats such a directory as the project root unless it is a declared workspace of an ancestor. Concretely, for a repo whose root has tooling dependencies but no workspaces field:

myapp/
  package.json          # no "workspaces"
  package-lock.json
  src/fn/package.json   # CodeUri, no lockfile

building src/fn in source returns myapp/package-lock.json, but npm running in src/fn resolves only src/fn/package.json and ignores that lockfile entirely. The workflow then picks NodejsNpmInstallAction for a project that has no lockfile of its own, dropping the deliberate npm update behaviour (re-resolve ranges, prune dependencies removed from the manifest, #579) for a reason that does not apply.

The same loop also has no upper bound: it keeps walking to the filesystem root, so a package.json plus package-lock.json anywhere above the build directory (a developer's home directory, a CI image root) silently selects the install strategy. That also makes the new TestNodejsNpmWorkflowGetLockfilePath cases that assert None depend on no such pair existing above the tempfile.mkdtemp() parent.

The distinguishing condition is whether the ancestor manifest declares the current directory as a workspace, which is the only reason npm resolves a lockfile from a parent:

while True:
    if osutils.file_exists(osutils.joinpath(current_dir, "package.json")):
        for lockfile_name in ("package-lock.json", "npm-shrinkwrap.json"):
            lockfile_path = osutils.joinpath(current_dir, lockfile_name)
            if osutils.file_exists(lockfile_path):
                return lockfile_path

        # this directory is npm's project root unless a parent declares it as a workspace,
        # in which case npm resolves the root lockfile from there
        if not _is_declared_workspace_of_parent(current_dir, osutils):
            return None

where the helper reads the parent manifest's workspaces entry (osutils.parse_json) and matches the relative path against its globs with fnmatch. That keeps the monorepo case working while bounding the search to directories that are genuinely part of the project.

[bnusunny]

Fixed in 9c5f169, and the walk is gone rather than repaired — npm can answer this itself.

Your claim checks out. I measured it on a myapp/ with package.json + package-lock.json (no workspaces) and a nested src/fn/package.json: installing in src/fn ignored the root lockfile and pulled lodash 4.18.1 while the root lockfile pinned 4.17.20. So returning that lockfile would have traded npm update away for nothing. The unbounded walk and the resulting test fragility were real too.

Rather than teach the walk to parse workspaces globs, get_lockfile_path() now asks npm prefix in the install directory and looks for a lockfile in exactly that one directory:

layout	npm prefix reports	lockfile used
workspace package (packages/fn)	monorepo root	root package-lock.json
plain nested package (src/fn)	src/fn itself	none — npm update

That is npm's own project-root resolution, so the answer is identical to the one the install acts on, and it cannot walk past the project into a home directory or CI image root. Measured identical on npm 8.19.4, 9.9.4, 10.9.9, 11.19.0. An npm prefix failure returns None, which keeps the previous npm update behaviour.

The TestNodejsNpmWorkflowGetLockfilePath cases now stub npm prefix instead of relying on what sits above tempfile.mkdtemp(), which removes the fragility you flagged. New case test_ignores_a_lockfile_outside_the_project_root pins your scenario: prefix = the nested package, root lockfile present, result NodejsNpmUpdateAction.

One note on the suggested _is_declared_workspace_of_parent helper: it would need to re-implement npm's workspace resolution (globs, nested workspace roots, workspaces.packages object form) and could drift from it. Asking npm is both smaller and authoritative, so I went that way.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..9c5f169
Files: 10
Comments: 1

[aws-sam-tooling-bot]

[GENERAL] The pruning behaviour of this new branch is untested. The PR description cites test_build_in_source_with_removed_dependencies as covering it, but that test builds tests/integration/workflows/nodejs_npm/testdata/npm-deps, which contains only included.js, excluded.js and package.json — no lockfile. The first build runs npm update --no-package-lock, which by design never writes a root package-lock.json (only node_modules/.package-lock.json, which get_lockfile_path does not look for), so the second build takes the NodejsNpmUpdateAction path again. That test therefore exercises only the unchanged no-lockfile branch.

Nothing in the suite covers the scenario this branch introduces: build in source, project has a lockfile, a dependency is removed from package.json, rebuild. That is exactly the case #579 was filed for, and it is the one behaviour the switch from npm update to npm install could regress — a stale lockfile still listing the removed package is now read on the second build.

A fixture with a lockfile plus a variant of the existing test would close the gap:

# source_dir fixture has a package-lock.json, so this build uses NodejsNpmInstallAction
# ... first build, assert minimal-request-promise present ...

shutil.copy2(
    os.path.join(self.temp_testdata_dir, "no-deps", "package.json"),
    os.path.join(source_dir, "package.json"),
)

# ... second build ...
self.assertNotIn("minimal-request-promise", set(os.listdir(source_node_modules)))

The new esbuild integration test does not help here: it asserts the locked version is installed, which covers the fix itself, not the pruning behaviour that was retained by keeping npm update for lockfile-less projects. The non-esbuild NodejsNpmWorkflow also has no integration coverage of this branch at all.

[bnusunny]

Fixed in 1ba2c01 — you were right that the PR description cited the wrong test, and the gap is now closed.

Both of your facts check out. testdata/npm-deps holds only included.js, excluded.js and package.json, and I confirmed separately that npm update --no-audit --no-save --omit=dev --no-package-lock --install-links leaves no root package-lock.json behind (only node_modules/.package-lock.json). So that test stays on the npm update branch on both builds, and citing it for the install branch was wrong. The description now cites each test for the branch it actually covers.

New test_build_in_source_with_removed_dependencies_and_a_lockfile covers the case you named, on the non-esbuild NodejsNpmWorkflow: fixture npm-deps-with-lockfile has a package-lock.json pinning minimal-request-promise 1.3.0 against a ^1.3.0 manifest, so the first build takes NodejsNpmInstallAction and the test asserts 1.3.0 landed; the second build, after no-deps/package.json replaces the manifest, asserts the package is gone. It fails on develop with AssertionError: '1.5.0' != '1.3.0', so it really does exercise this branch.

On the regression you were worried about — a stale lockfile still listing the removed package — the answer is that it does not happen: npm install --no-audit --no-save --omit=dev --install-links removes the package from node_modules while the lockfile keeps listing it, because the manifest decides what is extraneous and --no-save leaves the lockfile untouched. The new test now pins that, and it passes on npm 8.19.4, 9.9.4, 10.9.9 and 11.19.0.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..1ba2c01
Files: 15
Comments: 2

---

Comments on lines outside the diff:

[aws_lambda_builders/workflows/nodejs_npm/workflow.py:176] [GENERAL] Re-raising the artifact concern from the earlier review round — it was confirmed rather than dismissed, and the resolution was to move the test to the esbuild suite rather than address the behaviour.

Your own measurement of the workspaces-monorepo fixture through the non-esbuild NodejsNpmWorkflow produced artifacts/ = ['included.js', 'package.json'] with no node_modules, and require.resolve('minimal-request-promise') failing from the artifacts directory. The cause is that npm hoists to the monorepo root, so packages/fn/node_modules never exists, and LinkSinglePathAction then returns at LOG.debug level:

if not source_path.exists():
    # Source path doesn't exist, nothing to symlink
    LOG.debug("Source path %s does not exist, skipping generating symlink", source_path)
    return

The esbuild test added here passes because esbuild bundles the dependency into included.js, which masks the gap. For the plain npm workflow the result is a deployable package with no dependencies and no warning to the user — a failure that only surfaces at Lambda invocation time with MODULE_NOT_FOUND.

This is pre-existing rather than introduced here, but this PR makes workspaces monorepos a deliberately supported build-in-source path, so it is now in scope. At minimum, the missing node_modules in this configuration should be surfaced (a LOG.warning at the workflow level when building in source and the expected source node_modules is absent) rather than silently skipped. If you consider it out of scope, please say so explicitly and link a follow-up issue so it is not lost.

[aws-sam-tooling-bot]

[BUG] The new comment justifying why the npm ci branch stays gated on a lockfile in source_dir contradicts the branch added immediately below it:

# `npm ci` is deliberately gated on a lockfile in the source directory itself, and does not look for one
# in a parent directory: run from a workspace package, `npm ci` operates on the workspace root and prunes
# the root's dev dependencies, which would remove tooling the build itself relies on.

npm ci and npm install resolve the project root the same way (npm's local prefix), which is exactly what get_lockfile_path() relies on — its docstring states that npm prefix "reports the monorepo root for a workspace package." So when install_dir is a workspace package and the root lockfile is found, the new branch runs

npm install -q --no-audit --no-save --omit=dev --install-links

with cwd inside the workspace, and npm reifies the monorepo root tree with --omit=dev. The root's devDependencies are then not in the ideal tree and get pruned from the developer's own <monorepo-root>/node_modules — the same outcome the comment cites as the reason to avoid npm ci here.

Either the comment's rationale is wrong (in which case the npm ci gate deserves a different explanation), or the new branch needs the same gate. The new workspaces-monorepo fixture has no devDependencies at the root, so no test distinguishes the two cases. Worth verifying against a monorepo root that has root-level devDependencies before merging.

[bnusunny]

Comment corrected in 40ed159 — you are right that it was wrong, and the measurement reframes the rest.

I ran the exact scenario you asked for: a workspaces monorepo with a root devDependencies, a full npm install at the root first (so the dev dependency is present), then the install inside packages/fn. Control column is what develop runs today.

npm	install --no-save --omit=dev --install-links	update --no-save --omit=dev --no-package-lock --install-links (develop today)	npm ci --install-links
8.19.4	root devDep pruned	root devDep pruned	pruned
9.9.4	pruned	pruned	pruned
10.9.9	pruned	pruned	pruned
11.19.0	kept	kept	pruned

So the effect you describe is real on npm 10 and older, and it is --omit=dev reifying the workspace root — not something this branch introduces. npm update --omit=dev already does exactly the same thing on those versions, so the root's dev dependencies are pruned before and after this change alike. Dropping --omit=dev is what preserves them (verified: "kept" on all four), but that would ship dev dependencies into the artifact, so it is a separate behaviour decision rather than part of a lockfile fix.

What was genuinely wrong was my comment, which implied the pruning is peculiar to npm ci. The rewritten comment states what is measured: npm ci deletes node_modules and reifies from the lockfile, so it empties the root's dev dependencies on every version including npm 11, where the install leaves them in place; on npm 10 and older --omit=dev prunes them whichever command runs.

On the test gap: I left the fixture without root devDependencies on purpose. An assertion there would have to be version-conditional — pruned on npm ≤ 10, kept on npm 11 — which pins pre-existing --omit=dev behaviour rather than anything this PR decides, and would read as endorsing it. Happy to add it, or to open a separate issue about --omit=dev pruning a monorepo root on older npm, if you would rather have it tracked.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..40ed159
Files: 15
Comments: 2

[aws-sam-tooling-bot]

[GENERAL] The build-in-source path now runs a lockfile-reading npm install inside the developer's own tree, and nothing in the change establishes that npm leaves the source package-lock.json alone.

command = ["install", "-q", "--no-audit", "--no-save", "--omit=dev"]
if self.install_links:
    command.append("--install-links")

--no-save is documented as preventing writes to dependencies in package.json; it is not the same switch as --no-package-lock, and npm reifies the lockfile to match the tree it just built. The previous command was immune to this because --no-package-lock meant npm never touched the file. Two consequences specific to this branch, both landing in the source tree that this PR is trying to protect:

• with --omit=dev, the ideal tree npm reifies has no dev nodes, so a rewritten lockfile can come back without the developer's dev dependency entries;
• NodejsNpmLockFileCleanUpAction is deliberately skipped when build_dir == self.source_dir (workflow.py, _actions_for_cleanup), so whatever npm writes there persists across builds.

You measured the pruning and version-pinning behaviour of this command directly, so the same setup answers this: after test_build_in_source_with_removed_dependencies_and_a_lockfile's first build, is npm-deps-with-lockfile/package-lock.json byte-identical to the fixture? An assertion on that in the new integration test is cheap and pins the guarantee the PR is claiming. Note the current fixture has no devDependencies, so adding one would be needed to cover the --omit=dev case.

I could not verify npm's actual write behaviour here — the review is static only, so this is a request to confirm and lock in, not a claim that the lockfile is being clobbered.

[bnusunny]

Confirmed and pinned in 34e1339 — thanks for asking for the measurement rather than assuming either way.

--no-save does suppress the lockfile write, not just the package.json write. Measured with the exact command, sha256sum before and after, on all four npm majors CI builds, in the two shapes that matter:

case	npm 8.19.4	9.9.4	10.9.9	11.19.0
plain package, lockfile beside it	byte-identical	byte-identical	byte-identical	byte-identical
workspaces monorepo, root lockfile, install in packages/fn	byte-identical	byte-identical	byte-identical	byte-identical
lockfile pins 1.3.0, tree already holds 1.5.0, then install	byte-identical	byte-identical	byte-identical	byte-identical
dependency deleted from the manifest, lockfile still lists it	byte-identical	byte-identical	byte-identical	byte-identical

The last two are the cases where npm has a reason to rewrite: the lockfile disagrees with node_modules (the state develop leaves behind after one build), and it disagrees with package.json. In both, npm reifies the tree to match the lockfile and leaves the file alone — mtime unchanged too. Dev entries survive: after the install the lockfile still carries its "dev": true nodes even though --omit=dev kept them out of node_modules.

Your point about NodejsNpmLockFileCleanUpAction being skipped for the source tree is exactly why this is worth an assertion rather than a note, so both integration tests now read the lockfile bytes before the build and compare after:

• test_build_in_source_with_removed_dependencies_and_a_lockfile asserts it after the first build and after the second, where the manifest no longer lists the dependency. Its fixture gained an ms devDependency (with a "dev": true lockfile entry) so the assertion covers the --omit=dev case you named.
• test_esbuild_can_build_in_source_in_workspaces_monorepo_with_locked_versions asserts the monorepo root lockfile, the file the install never runs in but does reify.

Mutation-checked: dropping --no-save from the install command makes the plain test fail on the second build — npm writes a whole new lockfile there ("name": "nodeps", the dev entry gone). So the assertion pins the guarantee rather than restating a passing state.

Gates re-run on the new head: 958 unit+functional passed at 94.67% coverage, ruff and black --check clean, 275 nodejs + esbuild integration tests passed on npm 11, and the two tests above passed on npm 8.19.4, 9.9.4 and 10.9.9 as well.

[aws-sam-tooling-bot]

[GENERAL] Re-raising the artifact gap from the previous round: it was confirmed by your own measurement rather than declared acceptable, and the resolution was to move the test into the esbuild suite, which routes around the behaviour instead of recording it.

For a workspaces monorepo built in source through the plain nodejs_npm workflow, your measurement gave artifacts/ = ['included.js', 'package.json'] with no node_modules, and require.resolve('minimal-request-promise') failing from the artifacts directory. The mechanism is visible in the code: npm hoists to the monorepo root, so packages/fn/node_modules is never created, and _actions_for_linking_source_dependencies_to_artifacts links source_dir/node_modules, which LinkSinglePathAction.execute skips on a missing source with only a debug log:

if not source_path.exists():
    # Source path doesn't exist, nothing to symlink
    LOG.debug("Source path %s does not exist, skipping generating symlink", source_path)
    return

The result is a silently dependency-less Lambda package, with no warning at build time. This predates the PR and the diff does not make it worse — npm update hoisted to the same root. But this change is what makes monorepo builds a supported, tested shape, so the gap now sits directly behind the feature being added. Either raise the skipped link to a warning when building in source (so the failure is visible at build time rather than at invoke time), or link an issue tracking it so the esbuild-only coverage is a recorded limitation rather than an unexplained asymmetry between the two workflows.

[bnusunny]

Tracked as #933 — you are right that moving the test routed around the behaviour instead of recording it.

Re-measured both sides before filing, so the issue carries a control rather than a claim:

	this branch (34e1339)	develop (587257c)
artifacts/	['included.js', 'package.json']	['included.js', 'package.json']
artifacts/node_modules	absent	absent
packages/fn/node_modules	none (npm hoisted)	none (npm hoisted)
require.resolve from artifacts/	UNRESOLVABLE	UNRESOLVABLE
version at the monorepo root	1.3.0 (locked)	1.5.0 (upgraded)

Identical artifact, and only the version differs — which is the bug this PR fixes. So the gap is not reachable-only-now; npm update hoisted to the same root before.

On the two options you offered, I took the issue rather than the warning. Raising LinkSinglePathAction's missing-source debug log to a warning changes a shared action that four workflows call, and the same skip is the intended no-op elsewhere — nodejs_npm itself relies on it for the external-manifest link. A warning that is only correct for one caller belongs in the workflow, not the action, and deciding where the build should refuse versus warn is a behaviour change worth its own change rather than a rider on a lockfile fix. #933 has the mechanism, the code pointer, the repro and the measurement, so whoever picks it up does not have to re-derive any of it.

If you would rather see the warning land here, say so and I will add it to NodejsNpmWorkflow — the esbuild-only coverage is a recorded limitation now either way, not an unexplained asymmetry.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..34e1339
Files: 15
Comments: 1

---

Comments on lines outside the diff:

[aws_lambda_builders/workflows/nodejs_npm/workflow.py:176] [GENERAL] Re-raising the artifact gap from the previous two rounds. It was confirmed by your own measurement rather than declared acceptable, and the resolution — moving the monorepo fixture into the esbuild suite — routes around the behaviour instead of recording it.

For a workspaces monorepo built in source, npm resolves its project root to the monorepo root (the same resolution get_lockfile_path now relies on), so it hoists dependencies into /node_modules and never creates packages/fn/node_modules. _actions_for_linking_source_dependencies_to_artifacts then links source_dir/node_modules, and LinkSinglePathAction.execute returns silently when the source path is absent:

if not source_path.exists():
   # Source path doesn't exist, nothing to symlink
   LOG.debug("Source path %s does not exist, skipping generating symlink", source_path)
   return

Through the plain nodejs_npm workflow that yields artifacts/ without node_modules, matching what you measured (['included.js', 'package.json'], require.resolve('minimal-request-promise') failing). The new test only exercises the esbuild workflow, where the bundler inlines the dependency and the missing link cannot be observed — the require(bundle) assertion passes for a bundled import regardless.

I accept this hoisting predates the PR (npm update --no-package-lock --install-links resolves the same project root). The concern is that the PR adds the first workspaces-monorepo fixture and asserts it works, while the one workflow where it silently produces an incomplete artifact stays untested. Either add a plain-nodejs_npm test over workspaces-monorepo asserting current artifact contents (an xfail-style record is fine), or note the limitation next to the linking property so the gap is discoverable rather than implied to be covered.

[bnusunny]

Recorded in the code in 41f2ff4, which is the second of the two options you named.

_actions_for_linking_source_dependencies_to_artifacts now carries the gap where a reader meets the linking property:

# Known gap (aws/aws-lambda-builders#933): in an npm workspaces monorepo npm hoists to the
# monorepo root, so source_dir/node_modules is never created and LinkSinglePathAction skips
# silently, leaving the artifacts without dependencies. Pre-dates the lockfile lookup below and
# applies to this workflow only - the esbuild workflow bundles the dependency into the output.

#933 (filed before this round) carries the measurement, the mechanism, the code pointer and a repro. The PR description says the same thing in prose, so the limitation is discoverable from the code, the issue and the description rather than implied away by the new workspaces coverage.

I did not take the first option. An xfail-style test over workspaces-monorepo would assert artifacts/ == ['included.js', 'package.json'] and require.resolve failing — that pins the broken shape as the expected one, and #933's fix would then have to delete the test that recorded it. A comment plus an issue says the same thing without a test whose purpose is to be wrong.

Your reading of the esbuild assertion is right and worth stating plainly: require(bundle) passes because esbuild inlined the import, so it proves the dependency resolved at build time, not that the artifact carries node_modules. It is evidence for the lockfile fix (the bundle contains 1.3.0's code, which is why the test fails on develop with '1.5.0' != '1.3.0'), and it is not evidence about the linking property. Nothing in this PR claims the plain workflow's monorepo artifact is covered.

Gates on 41f2ff4: 958 unit+functional passed at 94.67% coverage, ruff and black --check clean, 275 nodejs + esbuild integration tests passed. The change is a comment, so no behaviour moved.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..41f2ff4
Files: 15
Comments: 1

[aws-sam-tooling-bot]

[GENERAL] The evidence that this install leaves the developer's lockfile untouched covers only lockfileVersion: 3 fixtures. Both new byte-identity assertions (test_build_in_source_with_removed_dependencies_and_a_lockfile, test_esbuild_can_build_in_source_in_workspaces_monorepo_with_locked_versions) use the two lockfiles added by this PR, and both are "lockfileVersion": 3.

A lockfile written by npm 6 (v1) or npm 7/8 (v2) is a different case: npm has to convert it in memory before it can reify, and format migration is the one situation where npm has a reason to persist the result. That case is not measured anywhere in this PR, and it is common in real projects — exactly the projects this change newly routes through a lockfile-reading install in their own source tree.

The repo already exercises it. tests/integration/workflows/nodejs_npm_esbuild/testdata/with-deps-esbuild/package-lock.json is:

{
 "name": "with-deps-esbuild",
 "version": "1.0.0",
 "lockfileVersion": 2,
 ...

and test_esbuild_can_build_in_source builds self.source_dir, which setUp points at the checked-in fixture rather than at temp_testdata_dir:

self.source_dir = os.path.join(self.TEST_DATA_FOLDER, "with-deps-esbuild")

Before this change that test ran npm update --no-package-lock, which could not touch the file. After it, the install reads and reifies from that v2 lockfile in place, and tearDown only removes node_modules — so if npm does write the migrated lockfile, the run silently modifies a tracked file in this repository, and the same write would land in a user's source tree on the first sam build --build-in-source.

Two cheap ways to pin it down, either of which settles the question for v1/v2 as the existing tests settle it for v3:

• add the same sha256/byte comparison of with-deps-esbuild/package-lock.json to test_esbuild_can_build_in_source (and point it at the temp copy, so a failure does not leave the fixture rewritten), or
• give one of the new fixtures lockfileVersion: 1 or 2 and keep the existing byte-identity assertion on it.

[bnusunny]

Good catch on the version gap — covered in 08597d0, and the migration write does not happen.

The workspaces-monorepo fixture's lockfile is now lockfileVersion: 2, so the two byte-identity assertions cover 2 and 3 between them, with no change to any pre-existing test. Its root entry still pins minimal-request-promise 1.3.0 against a ^1.3.0 manifest, so it keeps failing on develop for the original reason.

Measured first, since the answer decides whether a test or a fix was needed. Same command, sha256sum before and after, a fixture in each format:

fixture format	npm 8.19.4	9.9.4	10.9.9	11.19.0
lockfileVersion: 1	byte-identical, stays v1	byte-identical, stays v1	byte-identical, stays v1	byte-identical, stays v1
lockfileVersion: 2	byte-identical, stays v2	byte-identical, stays v2	byte-identical, stays v2	byte-identical, stays v2
lockfileVersion: 3	byte-identical	byte-identical	byte-identical	byte-identical

All twelve installed the locked 1.3.0, so npm did read and honour the v1 and v2 lockfiles — it converts for its own ideal tree and --no-save keeps the conversion from reaching disk. The version field is unchanged afterwards in every case, which is the direct answer to the migration-persist question.

On with-deps-esbuild you read the code correctly: its lockfile is v2 and setUp points self.source_dir at the checked-in fixture, not the temp copy. The write does not occur, though. After a full tests/integration/workflows/nodejs_npm tests/integration/workflows/nodejs_npm_esbuild run on this head — 275 passed — git status lists only the two files I edited; with-deps-esbuild/package-lock.json is unmodified. Same on the two earlier full runs.

I did not add the assertion to test_esbuild_can_build_in_source. Pointing it at temp_testdata_dir changes a pre-existing test's subject, and asserting against the checked-in path is the shape you rightly flagged as leaving a rewritten fixture behind on failure. Converting my own fixture covers the same format in a test whose subject is already a temp copy.

One limit worth stating: dropping --no-save from the install command kills test_build_in_source_with_removed_dependencies_and_a_lockfile on its second build, but not the monorepo test — npm leaves a workspace root's lockfile alone even without --no-save, at v2 as at v3. So the monorepo assertion documents the root-lockfile shape rather than killing that mutation; the plain test is the one that pins the guarantee.

Gates on 08597d0: 958 unit+functional passed at 94.67% coverage, ruff and black --check clean, 275 nodejs + esbuild integration passed on npm 11, and both byte-identity tests passed on npm 8.19.4, 9.9.4 and 10.9.9 with the v2 fixture.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..08597d0
Files: 15
Comments: 1

[aws-sam-tooling-bot]

[GENERAL] Re-raising the lockfile-format half of the previous round's concern, which is now partly but not fully covered.

The safety of running a lockfile-reading install inside the developer's own tree rests on --no-save suppressing the lockfile write. The monorepo fixture moved to "lockfileVersion": 2 in this revision, so the byte-identity assertions now cover v2 (test_esbuild_can_build_in_source_in_workspaces_monorepo_with_locked_versions) and v3 (test_build_in_source_with_removed_dependencies_and_a_lockfile). Neither covers lockfileVersion: 1, the format npm 6 wrote and which is still committed in older repos.

v1 is the one case where the write is not driven by a dependency change: npm 7+ has to migrate the format rather than just reify from it, and that migration is a separate write path from the one the v2/v3 measurements exercise. If npm does persist the migration under --no-save, the result is the exact harm this PR exists to prevent — a build silently rewriting a committed lockfile — and it would land only on projects too old to be represented in the fixtures.

Either add a v1 fixture to one of the two byte-identity tests, or post the same four-major measurement for a v1 lockfile so the gap is closed by evidence rather than left open.

[bnusunny]

The v1 measurement you are asking for is already posted — the table in my previous reply has lockfileVersion: 1 as its first row, byte-identical and still v1 on all four majors. Our comments crossed by five minutes, so here it is again with the sharper case added.

Your framing pointed at something my first probe did not isolate, so I ran it: a v1 lockfile that also disagrees with the manifest, giving npm a format migration and a dependency change as reasons to write at once.

v1 fixture (npm-6 shape: legacy dependencies block, no packages)	npm 8.19.4	9.9.4	10.9.9	11.19.0
manifest agrees	byte-identical, stays v1, installs 1.3.0	same	same	same
dependency removed from the manifest	byte-identical, stays v1, dependency pruned	same	same	same

The second row is the one that settles it. npm pruned minimal-request-promise from node_modules while leaving a v1 lockfile that still lists it completely untouched — so the migration is not persisted even when npm is simultaneously reifying a different tree than the lockfile describes. --no-save suppresses the write on the migration path, not only on the reify-identical path.

Both rows read the pin: 1.3.0 installed against a ^1.3.0 manifest in the agreeing case, which is only possible if npm read the v1 lockfile rather than re-resolving.

On the fixture half, I am not adding a third one, and this is a proportionality judgement rather than a disagreement about the risk. There are two byte-identity tests and three formats; converting either fixture to v1 trades away v2 or v3, and a third integration test exists only to walk the format axis — 20 measured combinations across two write pressures and four npm majors is stronger evidence about that axis than one more fixture pinned at whatever npm the lane happens to run. The formats that stay in the suite are the two a lockfile committed today actually has.

Say the word if you would rather have the fixture anyway and I will add it — it is a small test, and your call on your repo beats my read of the tradeoff. Head is unchanged at 08597d0; this needed no code change.

[bnusunny]

Added in bda8848 — and it earns its place, so I withdraw the proportionality argument I made against it.

npm-deps-with-v1-lockfile is a third fixture in the npm-6 shape (legacy dependencies block, no packages, a "dev": true entry for ms), and test_build_in_source_with_a_version_1_lockfile asserts the locked 1.3.0 is installed and the lockfile comes back byte-identical. The other two fixtures keep versions 2 and 3, so the three formats are covered by three tests rather than traded against each other.

The reason it was worth adding is a result my probe could not produce, because a probe only measures the shipped command:

mutation: drop --no-save from the install	npm 8.19.4	9.9.4	10.9.9	11.19.0
v1 fixture — new test	fails (lockfile rewritten v1 -> v3)	fails	fails	fails
v3 fixture — existing test	passes 1st build, fails 2nd	same	same	same
v2 monorepo fixture	passes	passes	passes	passes

So the migration path does write, on every major, on the very first build — --no-save is what suppresses it, and until now nothing in the suite would have noticed if that flag were dropped. The v1 test is the strictest of the three: it kills the mutation immediately, where the v3 test needs a second build with a drifted manifest and the v2 monorepo test does not kill it at all.

That does not contradict the measurements in my previous reply — those were all of the real command, with the flag, and every one still holds. What it changes is my reading of how much the flag is doing: on v2 and v3 npm leaves the file alone either way, so I treated --no-save as belt-and-braces there. On v1 it is the only thing standing between a build and a rewritten committed lockfile. Your instinct that migration is a separate write path was the right one to push on.

One property worth noting since you raised it for with-deps-esbuild: this test builds self.temp_testdata_dir, so the four mutation runs above left the checked-in fixture on disk still at version 1. A failure here cannot leave a rewritten fixture in the repository.

Gates on bda8848: 958 unit+functional passed at 94.67% coverage, ruff and black --check clean, 280 nodejs + esbuild integration passed on npm 11.19.0, and the lockfile subset passed on 8.19.4, 9.9.4 and 10.9.9 as well.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..bda8848
Files: 19
Comments: 1

[aws-sam-tooling-bot]

[GENERAL] The comment added just above this branch records a dev-dependency difference on npm 11 that the new branch inherits, and nothing in the change or the tests pins it down:

# it empties the workspace root's dev dependencies on every npm version, including npm 11 where the
# install below leaves them in place. (On npm 10 and older --omit=dev prunes them whichever command
# runs, exactly as npm update does today.)

Read literally, --omit=dev prunes already-installed dev dependencies for both commands on npm ≤ 10, and on npm 11 the install "leaves them in place" — which implies the removed npm update pruned them there and the new NodejsNpmInstallAction does not. That matters because build-in-source installs into the developer's own tree, which will normally already contain dev dependencies from their own npm install, and for the plain nodejs_npm workflow _actions_for_linking_source_dependencies_to_artifacts symlinks source_dir/node_modules straight into the artifacts. On npm 11 that would put dev dependencies into the deployment package, contradicting the LOG.debug a few lines above ("Dev dependencies are omitted from the Lambda artifacts package").

Neither new integration test exercises this: npm-deps-with-lockfile and npm-deps-with-v1-lockfile both declare a ms devDependency, but each test builds into a fresh copy where node_modules starts empty, so ms is never installed in the first place and its absence afterwards proves nothing. The only assertion about ms is that its lockfile entry is unchanged.

Worth either posting the npm 11 update-vs-install measurement for a tree where the dev dependency is already installed, or adding a test that runs a full npm install in the fixture first and then asserts what the build leaves in node_modules and in the artifacts. If dev dependencies do survive on npm 11, the comment should say so where it affects the artifact rather than as an aside justifying the npm ci gate.

[bnusunny]

Both halves addressed in d42b637: the comment was genuinely misleading, and the coverage gap you identified was real. The consequence you derived from the comment does not happen, though — measured below.

The comment described the monorepo root's dev dependencies and read as if it were a general statement about the install. Those are two different scopes with different answers:

dev dependency already installed, then the build runs	install --no-save --omit=dev (this PR)	update --omit=dev --no-package-lock (develop)
in the install directory itself — npm 8.19.4 / 9.9.4 / 10.9.9 / 11.19.0	pruned on all four	pruned on all four
at the monorepo root, install running inside packages/fn — npm 8 / 9 / 10	pruned	pruned
at the monorepo root — npm 11.19.0	kept	kept

The install directory's own dev dependencies — the ones the plain workflow symlinks into the artifacts — are pruned on every version by both commands. So npm 11 does not put dev dependencies in the deployment package, and the LOG.debug above still holds.

Where the reading went wrong is the contrast the comment was drawing. "The install below leaves them in place" on npm 11 is set against npm ci, which prunes the root's dev dependencies on every version; it was never a claim about update versus install. The bottom row is the check: on npm 11 npm update keeps the root's dev dependencies too, so that difference is not something this PR introduces either way. The comment now names the scope in both directions and says the root case is unchanged by the switch.

Your coverage point stands on its own and I have taken it: you are right that ms was never installed, so its absence proved nothing. test_build_in_source_drops_already_installed_dev_dependencies runs the developer's own npm install in the fixture first, asserts ms is present before the build, then asserts after it that ms is in neither node_modules nor artifacts/node_modules — and that the two directory listings are equal, since the artifact is a symlink to the same tree.

Two honest notes on what that test is and is not. Dropping --omit=dev from the install makes it fail ('ms' unexpectedly found), so it pins the guarantee. It also passes against develop's source, because npm update --omit=dev prunes identically — it is a pinning test for the artifact contract, not a regression test for this change, and I would rather say that than imply it catches something the base gets wrong.

Gates on d42b637: 958 unit+functional passed at 94.67% coverage, ruff and black --check clean, 285 nodejs + esbuild integration passed on npm 11.19.0, and the new test passed on 8.19.4, 9.9.4 and 10.9.9 as well.

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..73e9e1c
Files: 19
Comments: 1

[aws-sam-tooling-bot]

Code Review Results

Reviewed: 587257c..27db041
Files: 23
Comments: 1