Skip to content

Feat: declare a supported Node.js range (>=20.6.0) on the published packages and enforce it against the lockfile - #544

Open
AmaadMartin wants to merge 3 commits into
mainfrom
feat/declare-node-engines-published-packages
Open

Feat: declare a supported Node.js range (>=20.6.0) on the published packages and enforce it against the lockfile#544
AmaadMartin wants to merge 3 commits into
mainfrom
feat/declare-node-engines-published-packages

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A — no existing issue.
  2. Or, if no issue exists, describe the change:

Problem: none of the three published manifests declares an engines field,
so npm install @google/adk on an unsupported Node prints nothing and the
failure surfaces later and opaquely, from a transitive dependency. The only
support statement in the repo was prose at README.md:55"requires a current
Node.js LTS release"
— which no tool can check and whose meaning drifts with the
calendar (at time of writing that could mean 20.x, 22.x or 24.x).

Solution: three changes.

  1. "engines": {"node": ">=20.6.0"} on core, dev and integrations — the
    three release-please-managed, publishable workspaces.
  2. A check that the declared floor is still satisfied by every runtime
    dependency in the committed package-lock.json, so the declaration cannot
    quietly become a lie.
  3. The README prerequisite replaced with the concrete floor.

Why >=20.6.0, and why it is derived rather than chosen. The value is the
exact intersection of the engines.node ranges in the runtime dependency
closure, computed from the committed lockfile:

Node Runtime-closure dependencies that reject it
20.0.0 27
20.5.0 27 (@opentelemetry/*, ^18.19.0 || >=20.6.0)
20.6.0 0
22.12.0 0
24.0.0 0

>=20.0.0 (13 packages including @google/genai) eliminates the ^18.19.0
branch of the OpenTelemetry range, leaving >=20.6.0 as the tight bound.

The two ranges in the lockfile that would force a higher floor are
devDependency-only and must not be allowed to raise a consumer-facing
declaration: ^20.19.0 || ^22.13.0 || >=24 (five copies of
eslint-visitor-keys under typescript-eslint) and ^20.19.0 || >=22.12.0
(vite, via vitest). A consumer installing @google/adk never receives
either, so tracking them would lock out runtimes that work fine.

That point is load-bearing, because three sibling PRs on this fork each picked a
different value and none is the derived one. Running this check against each:

Value Proposed in Verdict
>=20 #508 Unsound — admits 20.0.0, rejected by 27 runtime deps
>=22.0.0 #133 Sound, but excludes the working 20.6.0–21.x window
>=22.12.0 #445 Sound, but derived from vite — a devDependency no consumer installs

The root manifest deliberately gets no engines field: it is the npm
workspace container, release-please-config.json marks it skip-changelog, and
it has no files array, so a declaration there reaches nobody. A test pins that
distinction so it is not "helpfully" re-added.

The check is deliberately one-sided, and the test says so in a comment. It
asserts the floor is not too low — the direction that breaks users — and does
not assert it is tight. Asserting tightness would force a manifest edit every
time an unrelated dependency loosens its own range, and would forbid a
maintainer from later tightening the floor as a support-policy decision (e.g.
when Node 20 ages out). The floor is pinned from above by the literal
EXPECTED_NODE_RANGE assertion, so the published value still cannot move
without an explicit, reviewed edit.

Dependency added: semver + @types/semver as root devDependencies. The
closure contains ranges like ^18.19.0 || >=20.6.0, 18 || 20 || >=22 and
6.* || 8.* || >= 10.*; hand-rolling range comparison would be a bug farm.
semver@7.8.4 was already in the tree as a transitive runtime dependency, so
importing it without declaring it would be a phantom dependency, and semver 7
ships no types of its own. Nothing new is downloaded — the lockfile diff is two
lines in packages[""].devDependencies plus the three engines blocks npm
writes for the workspace packages.

Relationship to the other open PRs (collision check). Open PRs on the fork
were enumerated before any code was written:

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 \
  --json number,title,headRefName

Three live PRs add engines to these three manifests: #445 (>=22.12.0 +
a manifest drift guard), #508 (>=20 + .nvmrc and CI pins) and #133
(>=22.0.0 + .nvmrc, CI pins, root engines). None of the three checks the
declaration against the lockfile, which is how the unsound >=20 in #508
survived review. This PR supersedes all three for the manifest half and adds
the enforcement none of them has; whichever lands first, the others should be
closed or narrowed to their CI/.nvmrc halves rather than merged alongside.
Adjacent PRs cleared by file list: #443 (README.md only — overlaps item 3
here), #507/#467/#345/#415/#338 (lockfile freshness under npm ci, a
different invariant), #428/#416/#406/#509/#510 (CI Node pinning only).

Deliberately out of scope, so the omissions are not read as oversights: a root
.npmrc with engine-strict=true (never published, so it does nothing for
consumers; it would make devDependency ranges hard install errors —
including eslint-visitor-keys's rejection of Node 22.0–22.12 — and must land
together with a CI Node pin), pinning node-version in the workflows, and
"private": true on the root manifest.

Not a breaking change. npm treats an unsatisfied engines range as an
advisory EBADENGINE warning, so installs that succeed today still succeed.
Only consumers who have opted into engine-strict themselves get a hard
failure, and only on runtimes that 13 of core's runtime dependencies —
@google/genai among them — already reject. The declaration reports a
pre-existing incompatibility rather than creating one.

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:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

One new file, tests/integration/packaging/engines_test.ts. It lives under
tests/integration/ because that is the only tests/ project wired into both
npm test and npm run test:coverage (the unit:integrations project defined
in vitest.config.ts is in neither), and because tests/integration/build_setup/
is the existing precedent for repo-level packaging checks. Paths are built with
path.join so it behaves on the Windows runner. No existing test was modified,
skipped, weakened or deleted.

$ npx vitest run --project integration tests/integration/packaging/engines_test.ts
 ✓ |integration| tests/integration/packaging/engines_test.ts (8 tests) 43ms
 Test Files  1 passed (1)
      Tests  8 passed (8)

Eight cases across three concerns:

  • %s declares the shared engines.node range (×3) — a single equality against
    the literal >=20.6.0 per workspace, which simultaneously asserts the field is
    present, that all three agree, and that the floor is exactly the derived value.
    it.each names the failing workspace, so a drift still reports which one moved.
  • leaves engines off the workspace root — pins the published-vs-container
    distinction.
  • reads runtime engine constraints out of package-lock.json — the vacuity
    guard.
  • %s declares a floor every runtime dependency accepts (×3) — the core check.

Proving the tests can fail. Every assertion was run against a mutated tree
and observed to fail; the tree was restored and re-verified green (8/8) after
each. The offender counts are exact — a wrong filter or a wrong satisfies call
changes them.

Mutation Result
core floor → ">=20.5.0" FAIL, 27 offenders — engines.node >=20.5.0 admits Node 20.5.0, which the listed runtime dependencies reject...: expected [ …(27) ] to deeply equal []
core floor → ">=18.0.0" FAIL, 53 offenders
core floor → ">=22.0.0" Lockfile check passes (0 offenders — one-sided by design); the literal assertion fails: expected '>=22.0.0' to be '>=20.6.0'. This is exactly why the literal case exists.
engines deleted from integrations/package.json FAIL: expected undefined to be '>=20.6.0', and integrations/package.json declares engines.node undefined, which is not a satisfiable semver range
core floor → "<0.0.0" (valid range, unsatisfiable → minVersion returns null) FAIL: core/package.json declares engines.node "<0.0.0", which is not a satisfiable semver range
root package.json given an engines block FAIL: expected { node: '>=20.6.0' } to be undefined
closure predicate node_modules/no_such_dir/ (empties the closure) FAIL: expected [] to not have a length of +0

The last one is the vacuity guard: without it the whole check passes trivially
if the lockfile scan ever silently returns nothing, which is how a check like
this rots — green forever while testing an empty set. The two expect.fail rows
cover both halves of the null-floor guard (field absent, and
present-but-unsatisfiable) rather than dereferencing a possibly-null SemVer.

Coverage: this change adds no lines under core/src, dev/src or
integrations/src, the only paths in the vitest coverage.include list, so the
thresholds are untouched and were not edited. Every branch of the new code is
exercised above: both outcomes of each of the three filter conditions, both
expect.fail paths, and both the empty and non-empty offender lists.

No any, no as any, no @ts-expect-error, no eslint-disable, no coverage
suppression anywhere in this diff. npx tsc --noEmit --pretty false reports
281 errors before this change and 281 after (pre-existing core/test/**
dual-identity errors between core/dist/types and core/src); 0 mention
the new file.

Manual End-to-End (E2E) Tests:

Machine: node -vv22.22.2, npm -v9.2.0.

  1. The lockfile is a faithful stand-in for what a consumer installs. The
    check reads engine ranges out of package-lock.json; if that metadata were
    stale the whole thing would be theatre. Every collected constraint was
    cross-checked against the real package.json on disk in node_modules after
    a clean npm install:

    lockfile constraints: 426 | verified against installed package.json: 401 |
    not on disk: 25 | mismatched: 0
    

    Zero mismatches. (The 25 are hoisted/deduped paths npm records but does not
    materialise at that exact location.)

  2. The declared value is verified, not asserted — the probe behind the table
    at the top:

    Node 20.0.0: 27 rejecting deps (first: node_modules/@opentelemetry/context-async-hooks ^18.19.0 || >=20.6.0)
    Node 20.5.0: 27 rejecting deps
    Node 20.6.0:  0 rejecting deps
    Node 22.11.0: 0 rejecting deps
    Node 24.0.0:  0 rejecting deps
    
  3. devOptional sensitivity, measured rather than assumed: switching the
    predicate from dev !== true to !dev selects the same 426 entries and
    yields the same 0 offenders, because npm writes devOptional instead of
    dev for dual-reachable packages. The stricter predicate is kept anyway —
    devOptional packages are runtime-reachable — and is called out here so a
    future reader does not "simplify" it into a behaviour change.

  4. The published manifests carry the field, and the values agree:

    $ npm pkg get engines.node -w core -w dev -w integrations
    {
      "@google/adk": ">=20.6.0",
      "@google/adk-devtools": ">=20.6.0",
      "@google/adk-integrations": ">=20.6.0"
    }
    
  5. Build, lint, format, license:

    $ npm run build                                              # exit 0
    $ npx eslint tests/integration/packaging/engines_test.ts     # exit 0, no output
    $ npx prettier --check tests/integration/packaging/engines_test.ts core/package.json \
        dev/package.json integrations/package.json package.json package-lock.json README.md
    All matched files use Prettier code style!
    $ bash scripts/check_license.sh
    ✅ All files have the correct license header.
    
  6. Regression: the build_setup integration suite, which performs real
    npm install/npm run build runs against these manifests and this lockfile,
    still passes:

    $ npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
     Test Files  1 passed (1)
          Tests  20 passed | 4 skipped (24)
    
  7. CI on this PR is green: run-tests passed on ubuntu-latest, windows-latest
    and macos-latest (they run npm run test:coverage, which includes the
    integration project and therefore this new test), plus the cross-language
    run-tests job and check-license. No workflow file needed to change.

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.
[x] 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.

Amaad Martin added 2 commits August 2, 2026 14:35
Consumers of @google/adk, @google/adk-devtools and @google/adk-integrations
had no machine-readable statement of which Node.js versions are supported.
The only support statement was prose in the README -- "a current Node.js LTS
release" -- which no tool can check and whose meaning drifts with the calendar.
A user on an unsupported runtime got silence at install time and an opaque
failure later, from a transitive dependency.

The floor is derived from the runtime dependency closure recorded in
package-lock.json rather than chosen as a policy: across the 426 third-party
entries a consumer inherits, 27 @opentelemetry/* packages require
'^18.19.0 || >=20.6.0' and 13 more (including @google/genai) require '>=20.0.0',
so Node 20.5.0 is rejected and 20.6.0 satisfies every range. The two ranges in
the lockfile that would force a higher floor -- eslint-visitor-keys and vite --
are devDependency-only and are never installed by a consumer, so they must not
raise the published floor.

The root manifest deliberately gets no engines field: it is the npm workspace
container, release-please marks it skip-changelog, and it has no files array,
so a declaration there would reach nobody.

Not a breaking change. npm treats an unsatisfied engines range as an advisory
EBADENGINE warning, so installs that succeed today still succeed; only
consumers who have opted into engine-strict themselves see a hard failure, and
only on runtimes 13 of core's dependencies already reject.
Declaring a floor is only half the job: nothing checked that the floor is
actually deliverable. A floor that is too low promises support the packages
cannot honour, which is strictly worse than declaring nothing -- the install
looks clean and the failure surfaces later, from a transitive dependency, on a
runtime the metadata said was fine.

This walks every third-party entry in the committed package-lock.json that a
consumer inherits (dev entries dropped, devOptional kept, workspace entries
excluded since their range is the value under test) and asserts the lowest
Node version each published manifest admits satisfies all of them, naming the
offending package and range when it does not. A vacuity guard fails the run if
the lockfile scan ever returns nothing, so the check cannot rot into a no-op.

The check is deliberately one-sided: it pins the floor from below only, so a
dependency loosening its own range never forces a manifest edit, and a
maintainer stays free to tighten the floor later as a policy decision. The
literal range assertion pins it from above, so the published value cannot move
without an explicit, reviewed edit.

semver and @types/semver become root devDependencies: the ranges involved
('^18.19.0 || >=20.6.0', '18 || 20 || >=22') need a real range parser, and
semver@7.8.4 was already in the tree only as a transitive runtime dependency,
so importing it without declaring it would be a phantom dependency.
@AmaadMartin
AmaadMartin force-pushed the feat/declare-node-engines-published-packages branch from 730ccb3 to 3abdf46 Compare August 2, 2026 21:37
@AmaadMartin AmaadMartin changed the title Test: enforce the declared engines.node floor against the lockfile (stacked on #445) Feat: declare a supported Node.js range (>=20.6.0) on the published packages and enforce it against the lockfile Aug 2, 2026
@AmaadMartin
AmaadMartin changed the base branch from feat/declare-engines-node-range to main August 2, 2026 21:37
@AmaadMartin AmaadMartin closed this Aug 2, 2026
@AmaadMartin AmaadMartin reopened this Aug 2, 2026
The comment justified the assertion with two claims the repository
contradicts: release-please-config.json does register the root as a tracked
package (component "main"; only skip-changelog is set), and a missing files
array widens what npm publish includes rather than preventing publication.

Replaced with the reason that actually holds. The assertion itself is
unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant