Skip to content

Feat: declare a supported Node.js floor via engines and pin the CI matrix - #593

Open
AmaadMartin wants to merge 2 commits into
mainfrom
feat/node-engines-floor-ci-matrix
Open

Feat: declare a supported Node.js floor via engines and pin the CI matrix#593
AmaadMartin wants to merge 2 commits into
mainfrom
feat/node-engines-floor-ci-matrix

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 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: The repository publishes three npm packages (@google/adk,
@google/adk-devtools, @google/adk-integrations) but never states which
Node.js runtimes it supports in a form any tool can check.

  • No engines field in the root manifest or any of the three published
    workspace manifests (grep -rn '"engines"' --include=package.json returned
    nothing outside node_modules).
  • .github/workflows/validation.yaml:24 and
    .github/workflows/cross-language-integration.yml:18 both called
    actions/setup-node@v6 with no with: block, so every job silently ran
    on whatever Node the runner image happened to ship.

Two consequences. Consumers got no EBADENGINE signal at install time, so an
unsupported runtime surfaced later as an unexplained TypeError on a missing
built-in. And nothing in the repo exercised a declared floor, so a change
relying on a newer Node API could go green in CI and still break a supported
consumer.

Solution: Turn the README's prose into a machine-checkable contract.

  1. Declare "engines": {"node": ">=22.0.0"} in the root manifest and all three
    published workspace manifests, byte-identical.
  2. Add a node: ['22', '24'] dimension to the validation matrix and pass it
    to setup-node; pin the cross-language workflow to the floor ('22').
  3. Add one repo-config test that binds the manifests, the workflows, and the
    matrix together so they cannot drift apart.
  4. Point the README at engines.node and document the bump policy in
    CONTRIBUTING.md.

Why 22, and why a bare >= floor. Every value here is derived from a source
of truth in the repo or from the official release schedule, not from memory:

  • Node 20 is EOL. nodejs/Release schedule.json gives v20 Iron
    "end": "2026-04-30" and v18 Hydrogen "end": "2025-04-30". The LTS lines
    still in support are 22 (end: 2027-04-30) and 24
    (end: 2028-04-30), so 22 is the floor consistent with the README's existing
    "a current Node.js LTS release" wording.
  • The dependency graph already requires ≥20. From package-lock.json:
    @google/genai@2.9.0 >=20.0.0, commander@14.0.3 >=20,
    secretlint@11.7.1 >=20.0.0, lint-staged@16.4.0 >=20.17. A >=18
    declaration would have been false on day one.
  • >=20 would have a hole in it. vitest@3.2.6 declares
    ^18.0.0 || ^20.0.0 || >=22.0.0 — it skips Node 21. A >=20.0.0 floor
    nominally admits a runtime vitest refuses; >=22.0.0 does not.
  • Deliberately not a compound range. ^22 || ^24 would reject Node 26 when
    it becomes LTS on 2026-10-28 and would need an edit every release. A bare
    >= floor plus the documented bump policy is the intent.

Note for reviewers who remember #526. That PR removed a hard-coded Node
version from the README on the grounds that it goes stale. This change respects
that outcome rather than re-litigating it: no number is added to the README,
which keeps its version-free wording and only gains a pointer to
engines.node. CONTRIBUTING.md likewise states the rule ("the floor tracks
the oldest LTS line still in support") and no version. The concrete number lives
only where it must be machine-readable, and the new test binds those places
together so there is effectively one source of truth.

Collision check (required by our process, recorded here). Ran
gh pr list --state open --limit 1000 (490 open PRs) and diffed every plausibly
adjacent one. This area is heavily contested — 10 open PRs touch it: #571,
#572, #574, #508, #509, #510, #544, #549, #445, #133. The closest is #571
(feat/engines-node-floor-ci-matrix), which covers the same surface. I did not
stack on it because this change revises the lines it adds rather than building
on them, and the differences are substantive:

#571 This PR
Floor >=20.19.0an EOL line, and it pins a 20.19.0 CI leg, so CI actively tests an unsupported runtime >=22.0.0, the oldest LTS still in support
Root manifest omitted included
Guard test asserts only that the 3 manifests agree with each other also catches a floating setup-node, a workflow pinned below the floor, and matrix drift
Docs none README pointer + CONTRIBUTING bump policy

Maintainers should land one of these and close the rest.

Breaking change disclosure — this is not "metadata only." Declaring
engines changes install behaviour for consumers below the floor:

Package manager Behaviour below engines.node
npm (default) EBADENGINE warning; install proceeds
npm with engine-strict=true Install fails (opt-in; no .npmrc shipped)
Yarn Classic (v1) Install fails unless --ignore-engines
Yarn Berry / pnpm Warning by default; error under strict settings

README.md documents yarn add @google/adk, so for a Yarn 1 user on Node 20
this converts a working install into a hard failure. That is the intended
enforcement — Node 20 has been EOL since 2026-04-30 — but it is a real
behaviour change, not a no-op.

Considered and rejected:

  • .npmrc with engine-strict=true — turns a warning into a hard failure
    for every contributor and consumer. That is a much larger policy decision than
    this change carries. The CI matrix is the enforcement mechanism here: it
    actually runs the suite on the floor.
  • node-version-file: package.jsonsetup-node resolves a semver range
    to the newest satisfying version, so ">=22.0.0" would install the latest
    Node and the floor would never be exercised. Explicit majors are required.

Lockfile. package-lock.json is touched only because npm mirrors manifest
fields into its lockfile entries: the four engines blocks, plus the one
js-yaml declaration below. No package was installed, removed, or
version-bumped.

Phantom dependency fixed (second commit). The new test imports js-yaml
from tests/, but the root manifest declared only @types/js-yaml — the
runtime package resolved purely by npm hoisting it out of core/package.json
and dev/package.json. That would break if either workspace dropped it, or
under any strict / isolated node_modules layout. It is now declared
explicitly at the root at ^4.1.1, matching the range core and dev
already use. Worth fixing here specifically, since the premise of this PR is
making the repo's runtime contract explicit rather than incidental. Node ships
no YAML parser, so the dependency itself is warranted — only its declaration
was missing.

Two notes for maintainers before merging:

  1. Required-check names change. Adding a matrix dimension renames the jobs
    from run-tests (ubuntu-latest) to run-tests (ubuntu-latest, 22), and so
    on. If branch protection pins the old names as required checks, they will
    stop reporting and PRs would silently lose that gate until the rules are
    updated. Please update branch protection alongside this merge.
  2. The matrix re-runs some OS/Node-independent steps. secretlint, lint,
    format:check and docs:check now execute in six jobs instead of three.
    Narrowing that with matrix.include would make the YAML longer and would
    break the straightforward matrix assertion in the test, so the uniform
    cross-product is kept as the simpler expression.

On keeping the README pointer version-free. It was suggested that the two
added README lines either fold the number in directly (requires Node.js 22 or newer) or be dropped. Keeping the indirection is deliberate: writing the number
into the README is exactly what was reverted in #526 ("any hard-coded minimum
version goes stale over time"), and the new test cannot guard prose, so a number
there would be the one claim in this change with nothing enforcing it. The
pointer tells a reader where the authoritative, enforced value lives without
creating a second source of truth that can drift.

Out of scope (each is a separate risk class and is left alone deliberately):
@types/node is still pinned at ^20.12.7 in the root and dev manifests;
bumping it to ^22 is a major typings upgrade that can force production-source
changes, so it is not bundled here.

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.

Added tests/integration/repo_config/node_engines_test.ts (5 tests), placed
under tests/integration/** so it is picked up by the integration project,
which npm run test:coverage — and therefore the validation workflow —
actually runs. It asserts the five invariants:

  1. engines.node is present and identical in the root manifest and in every
    directory listed in the root workspaces array (the workspace list is read
    from the manifest, not hard-coded, so a new workspace is covered
    automatically).
  2. The running Node satisfies the declared floor.
  3. Every actions/setup-node step in every workflow pins an explicit
    node-version.
  4. No workflow pins a literal node-version below the floor (matrix references
    containing ${{ are skipped — they are covered by invariant 5).
  5. The validation matrix exercises every supported LTS line.

ENGINES_NODE_RANGE is derived from SUPPORTED_NODE_MAJORS[0], so the two
constants cannot disagree with each other.

$ npx vitest run --project integration tests/integration/repo_config/node_engines_test.ts
 ✓ |integration| tests/integration/repo_config/node_engines_test.ts (5 tests) 17ms
 Test Files  1 passed (1)
      Tests  5 passed (5)

Proof each test can fail. Every test was run against mutated code and
confirmed to FAIL. Each mutation was reverted afterwards:

# Mutation Observed failure
M1 Delete the engines block from integrations/package.json AssertionError: integrations/package.json engines.node: expected undefined to be '>=22.0.0'
M2 Root engines.node">=20.0.0" AssertionError: ./package.json engines.node: expected '>=20.0.0' to be '>=22.0.0'
M3 Remove with: node-version from cross-language-integration.yml AssertionError: cross-language-integration.yml job run-tests: setup-node must pin node-version: expected undefined to be truthy
M4 Validation matrix → node: ['24'] AssertionError: expected [ '24' ] to deeply equal [ '22', '24' ]
M5 Cross-language pin → '20' (below floor) AssertionError: cross-language-integration.yml job run-tests: node-version 20: expected 20 to be greater than or equal to 22
M6 SUPPORTED_NODE_MAJORS[0]'99' (above the running runtime) AssertionError: expected 22 to be greater than or equal to 99

M3 is the important one: it restores the exact defect this change exists to
fix — a floating setup-node — and proves the test would have caught it.

Coverage. vitest.config.ts restricts coverage.include to core/src/**,
dev/src/**, integrations/src/**. This change adds zero lines under those
paths, so the configured thresholds are unaffected and no threshold was edited.
The new test file executes every one of its own lines on every run.

Manual End-to-End (E2E) Tests:

Run from the repository root on Node v22.22.2:

npm install          # no EBADENGINE warning; confirms the floor admits the toolchain
npm run build        # pass
npm run lint         # pass
npm run format:check # pass — "All matched files use Prettier code style!"
npm run docs:check   # pass
bash scripts/check_license.sh   # "✅ All files have the correct license header."
npx secretlint "**/*"           # clean
npx vitest run --project integration tests/integration/repo_config/node_engines_test.ts

To confirm the four manifests agree, and see the consumer-facing value:

$ node -e "for (const p of ['.','core','dev','integrations']) console.log(p, JSON.stringify(require('./'+p+'/package.json').engines))"
.            {"node":">=22.0.0"}
core         {"node":">=22.0.0"}
dev          {"node":">=22.0.0"}
integrations {"node":">=22.0.0"}

CI result on this PR. All six matrix jobs pass (3 OS × Node 22 and 24),
plus the cross-language run-tests job:

run-tests (ubuntu-latest, 22)   pass      run-tests (ubuntu-latest, 24)   pass
run-tests (macos-latest, 22)    pass      run-tests (macos-latest, 24)    pass
run-tests (windows-latest, 22)  pass      run-tests (windows-latest, 24)  pass
run-tests (cross-language)      pass      check-license                   pass

The new test passed in every job, on both majors and all three OSes.

Getting there took re-runs, and the reason is worth flagging to maintainers
because this change makes it more visible, not less:

  • core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdout times out at 5000ms on windows-latest. It is a
    real-subprocess spawn racing a 5s budget, it is unrelated to this diff (which
    touches no source that test exercises), and Fix: give the real-subprocess cases in unsafe_local_code_executor_test.ts an explicit 60s timeout #498 is already open to give
    those cases an explicit 60s timeout.
  • tests/integration/app_loader/app_loader_test.ts flaked once on
    macos-latest, then passed.
  • For reference, the base commit b390217e already fails windows-latest
    validation on main
    (run 30669370416, adk_web/webui_test.ts) with none of
    these changes present.

I confirmed Node 24 itself is not the problem: #428 pins the identical
node: v24.18.0 on windows-latest and its job passes. So the retries were
genuine flakiness, not a Node 24 incompatibility introduced here.

One observation, left unfixed deliberately: this change takes the matrix from 3
jobs to 6, and the matrix still uses the default fail-fast: true. A single
flaky leg therefore cancels the other five, which is why windows-latest, 22
reported cancelled rather than a result on several attempts. Adding
fail-fast: false would make the wider matrix considerably more informative,
but #235 is already open for exactly that, so it is left out of this diff
rather than duplicated.

Known pre-existing failure, not introduced here: npm run ts:check reports
281 errors. That count and the exact set of failing files are identical on the
base commit
with this change stashed, and none of them is in the new file
(grep -c node_engines_test → 0). ts:check is not run by the validation
workflow. Several open PRs address it separately; it is out of scope here.

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 3, 2026 14:10
…trix

The repository published three npm packages without stating which Node.js
runtimes they support in any machine-checkable form. The only statement was
prose in the README, and both workflows called actions/setup-node with no
node-version, so CI silently ran on whatever Node the runner image shipped.

Declare "engines": {"node": ">=22.0.0"} in the root manifest and all three
published workspace manifests, pin the validation matrix to the supported LTS
lines ('22', '24'), and pin the cross-language workflow to the floor.

The floor is 22 because Node 20 Iron reached end of life on 2026-04-30, so 22
and 24 are the LTS lines still in support. It is also the lowest value with no
gaps: vitest@3.2.6 declares ^18.0.0 || ^20.0.0 || >=22.0.0, so a >=20 floor
would nominally admit a Node 21 that vitest refuses.

A repo-config test binds the three declarations together so they cannot drift:
it fails if the manifests disagree, if any setup-node step stops pinning a
node-version, if a workflow pins below the floor, or if the matrix stops
exercising every supported line.
…ting

The new repo-config test imports js-yaml from tests/, but the root manifest
declared only @types/js-yaml. The runtime package resolved purely by npm
hoisting it out of core and dev, so the import would break if either workspace
dropped the dependency, or under any strict or isolated node_modules layout.

Declare it explicitly at the root, matching the ^4.1.1 range core and dev
already use. No new package is installed; the lockfile delta is the
declaration mirror only.
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