Ci: assert declared production dependencies survive npm ci --omit=dev - #651
Open
AmaadMartin wants to merge 4 commits into
Open
Ci: assert declared production dependencies survive npm ci --omit=dev#651AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
August 4, 2026 11:53
A lockfile can be self-consistent and still resolve a published package's runtime dependency to a node that a production install prunes, so the package installs fine for contributors and fails at first import for consumers. Add a dependency-free check that reads every published workspace manifest and probes the installed tree for each declared dependency, plus its black-box test suite. The check is driven off the manifests rather than an allowlist, so it covers new dependencies automatically.
`run-tests` installs devDependencies, so a package reachable only through the dev tree still resolves there. Add a separate ubuntu-only job that performs a real production install and runs the check against it, which is the only place the published dependency set is exercised.
A published workspace may legitimately carry no runtime dependencies, and the fallback for a missing dependencies block was the one expression the suite never executed.
…ript npm already performs this check: `npm ls --omit=dev --workspaces --include-workspace-root` walks every workspace's declared dependencies against the installed tree and exits ELSPROBLEMS when one is absent. It also names the version range and the requiring package, and expands globbed workspace entries, which the hand-rolled script could not. Verified against a production install of this repo: exit 0 on a healthy tree; exit 1 naming the package when a declared dependency is deleted; exit 1 when a lockfile node for a declared production dependency carries "dev": true and npm ci --omit=dev prunes it, which is the incident this job exists to catch. Also revert an unrelated re-quoting of NODE_OPTIONS that a formatter applied to the env block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem: Nothing in CI ever performs a production install.
run-testsrunsnpm install, which installsdevDependenciestoo, so a package the publishedcode imports at runtime still resolves there even when a production install
would prune it. A lockfile can be perfectly self-consistent — and pass a
lockfile drift guard — while resolving a published package's runtime dependency
to a node that
npm ci --omit=devremoves. The end-user symptom is that thepackage installs fine for contributors and throws on first
importforconsumers.
Solution: Add one
production-installjob tovalidation.yaml:npm lsis the assertion, not a listing: it walks each workspace's declareddependenciesagainst the installed tree and exits non-zero withELSPROBLEMSwhen one is absent.
--workspaces --include-workspace-rootcovers all threepublished manifests plus the root. The check is manifest-driven, so it grows
automatically as dependencies are added, and there is no allowlist and no
package name anywhere in the diff.
This is a revision. The first version of this PR hand-rolled the same check as a
144-line
scripts/check_production_install.mjswith a 248-line test suite; areviewer pointed out that npm already does it. Both files are deleted here — the
diff is now 24 added lines in one file, and those tests go away only because
their subject does, not because coverage was thinned.
npm lsis also strictlybetter on three axes the script gave up on: it names the version range and the
requiring package rather than two probed paths, it expands globbed
workspacesentries instead of refusing to run, and
--include-workspace-rootcovers theroot manifest's own
dependencies(empty today, so latent).Two details that are load-bearing and easy to get wrong:
--ignore-scriptsis required, not defensive. Without it,npm ci --omit=devfails withcode 127 / command sh -c husky: the rootpreparescript runs husky, which is a devDependency and is not installedhere. It also keeps arbitrary postinstall code from running and keeps the job
fast, and it does not weaken the assertion, since every package is still
extracted.
ubuntu-latestonly, deliberately. The declared dependency set isplatform-independent, so a three-OS matrix would triple the cost for no extra
signal. This is a choice, not an oversight.
Honest limits.
srcimports but that appears in no manifest, or only indevDependencies, is not part of the tree npm walks, so this job stays green.Catching that needs an import-graph scan, which is separate work and out of
scope. Calling this job "catches undeclared runtime imports" would be false.
npm lsis stricter than "is every declared dependency installed": on a--omit=devtree it also reportsinvalid,extraneousand unmet peers asELSPROBLEMS. That is stricter in the right direction for a production-installgate, and it is quiet on this tree today — exit 0 both locally and on the CI
runner. If it ever turns noisy, that is the fact that would justify bespoke
code; it is not true now.
::error::annotationin the GitHub UI (the failed step and the
npm ERR!lines remain), and a"verified N dependencies" summary line.
npm lsprints the resolved treeinstead, which is more informative about what was actually checked.
Deliberate scope limits (nothing silently dropped): no dependency
declaration or lockfile change; no lockfile-drift step; no Node version pin, npm
cache, or concurrency group on the workflow; no import-graph scan; no npm script
alias.
Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 550 open PRs; I also fetched every fork branch and grepped all of them
for
omit=dev,--production,check_production_installandproduction-installunder.github/workflows/andscripts/. No branchperforms a production install. The nearest neighbours, none of which this
duplicates or conflicts with:
fix/lockfile-adm-zip-dev-flag) asserts the same invariantstatically, by reading
package-lock.jsonfor a non-dev node per productiondependency. That is a model of the install; this job is the install. Disjoint
files, so I branched from
mainrather than stacking, which would havedragged that PR's
package-lock.jsonedit into this diff.check_phantom_deps.mjs) covers the opposite direction(import -> manifest). No conflict.
run-testsjob. This PR appends a new top-level job at the end of the fileand does not touch
run-tests, so the hunks are far apart and merge in eitherorder.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[ ] I have added or updated unit tests for my change.
[ ] All unit tests pass locally.
There are no unit tests, and that is the point of the revision: the change is a
workflow job invoking npm, with no repo code left to test. The job runs on every
PR, so CI exercises the real thing; a test asserting that
npm lsbehaves likenpm lswould only pin a mock. The change touches no source, so the existingsuites are unaffected. What I did instead is verify the command empirically
against a real production install of this repo, in a scratch clone (
npm cideletes
node_modules, so do not do this in your working checkout). Theabsolute clone path is abbreviated to
<clone>below.1. Healthy production install — exit 0.
All 49 declared production dependencies are listed, including the two workspace
links that resolve through a symlink (
@google/adk -> ./core, declared by bothdevandintegrations), and no devDependency is needed for the command tosucceed. No
invalid,extraneousorUNMETlines.2. A declared dependency deleted — exit 1, named.
Both failures are reported in one run, each naming the version range and the
requiring workspace.
3. The actual incident class — exit 1. This is the shape of the
adm-zipbug that motivates the job, and the case a lockfile drift guard does not catch:
a manifest declares the package under
dependencieswhile its lockfile nodecarries
"dev": true, so the lockfile stays self-consistent andnpm cisucceeds but prunes the package. Reproduced by injecting
"dev": trueonto thenode_modules/jsonpath-pluslockfile node in the scratch clone:4.
--ignore-scriptsis necessary — dropping it fails the install beforethe check can run:
5. On the CI runner (Node v22.23.1, ubuntu-latest). The new job on this PR's
own run:
npm ci --omit=dev --ignore-scriptsreported "added 598 packages, andaudited 602 packages in 12s", then
npm ls --omit=dev --workspaces --include-workspace-rootprinted the resolvedtree from
adk@1.5.0 /home/runner/work/adk-js/adk-jswith noELSPROBLEMS,invalid,extraneousorUNMETlines, and the job passed in 19s. That is theevidence for the "stricter, but quiet on this tree" claim above, on the npm
version CI actually uses rather than only the one on my machine.
One unrelated red on this branch:
run-tests (windows-latest)failed once onapp_loader_test.ts > should discover apps vs agents across directories and standalone fileswithTest timed out in 40000ms. That suite is untouched bythis diff (which is one workflow file), it is a known intermittent timeout on
the Windows and macOS runners with open fixes of its own, and it passed on
re-run. All checks are green now.
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
The four blocks above are the manual test, each reproducible with the commands
shown. The job on this PR's own CI run is the same sequence on a clean runner;
its result is visible in the checks on this PR.
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[ ] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.