Skip to content

chore: declare supported Node version and pin it in CI - #572

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/pin-ci-node-version
Open

chore: declare supported Node version and pin it in CI#572
AmaadMartin wants to merge 2 commits into
mainfrom
fix/pin-ci-node-version

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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Problem:

The repository never states which Node.js runtime it supports, and CI never selects one.

Both workflows that run JavaScript call actions/setup-node with neither node-version nor node-version-file:

File Step
.github/workflows/validation.yaml - name: Use Node.js / uses: actions/setup-node@v6
.github/workflows/cross-language-integration.yml - name: Use Node.js / uses: actions/setup-node@v6

These are the only two setup-node call sites (grep -rn "setup-node" .github/ returns exactly those two). node-version has no default: with nothing supplied the action installs nothing and every later run: step executes on whatever Node the runner image happens to ship.

Nothing else pins it either — before this change, grep -rn '"engines"' --include=package.json . | grep -v node_modules returned nothing across all four manifests, and there is no .nvmrc or .node-version.

Four consequences, all live before this PR:

  1. The tested runtime drifts with the runner image, with no commit in this repository.

  2. Different legs of the same matrix run different Node majorsubuntu-latest, windows-latest and macos-latest are independently versioned images, so a green matrix does not mean "green on Node X". This is not hypothetical; it is observable in this repository's own CI right now. A recent validation run on an unrelated branch that still has the bare setup-node step reported:

    run-tests (ubuntu-latest)   node: v22.23.1
    run-tests (windows-latest)  node: v22.23.1
    run-tests (macos-latest)    node: v24.18.0     <-- different major, same matrix
    

    Two Node majors in one green matrix, chosen by the image rather than by this repository.

  3. The type surface and the executed runtime are decoupled. The repo pins @types/node: ^20.12.7 — code is type-checked against the Node 20 API — while tests execute on an unrelated, unstated major.

  4. Consumers are told nothing. @google/adk, @google/adk-devtools and @google/adk-integrations shipped with no engines field, so npm could not warn a user installing on an unsupported Node.

Solution:

Two declarations, seven files, 28 added lines. No TypeScript, no runtime code, no new files.

A. engines.node: ">=20.19.0" — added identically to the root manifest and to each of the three independently published workspaces. Each workspace has its own files array containing package.json and its own prepublishOnly, so each must carry the declaration for it to reach consumers.

The floor is measured, not guessed. Evaluating every engines.node range in package-lock.json against candidate versions with semver.satisfies:

Candidate Lockfile packages whose engines.node is unsatisfied
20.0.0 74
20.11.0 11
20.17.0 10
20.19.0 0
22.0.0 6
24.0.0 0

The binding constraints at the top of the Node 20 line are vite@7.3.5 (^20.19.0 || >=22.12.0) and eslint-visitor-keys@5.0.1 (^20.19.0 || ^22.13.0 || >=24), pulled in transitively by vitest and typescript-eslint. 20.19.0 is the lowest version at which the current tree is fully satisfied, so it is the lowest honest value. It is also still within the Node 20 major that @types/node: ^20.12.7 pins.

A floor rather than an exact pin is deliberate: engines is a consumer-facing compatibility statement. "20.x" there would tell every user on Node 22/24 they are unsupported. The floor leaves them supported; the CI pin below is what makes the tested version deterministic.

B. node-version: '20.x' — added to the existing setup-node step in both workflows. Nothing else about either step changed, and the action version was not bumped. The value is quoted so YAML reads it as a string (node-version: 20.10 unquoted parses as the float 20.1). '20.x' pins the major and lets the patch float to the newest Node 20, currently 20.20.2, which satisfies the declared >=20.19.0 floor.

Why not node-version-file: pointing it at package.json makes the action resolve engines.node, which is the range >=20.19.0. With the default check-latest: false the action first checks the local tool cache for a semver match, so the image's newer cached Node satisfies the range — reproducing exactly the drift this PR removes. Making node-version-file actually pin would require narrowing engines.node to "20.x", which breaks the consumer contract above. Hence: engines = consumer floor, node-version = CI pin.

Deliberately out of scope (each would turn a reviewable chore into a behaviour change): no .nvmrc / .node-version (a third place for the version to live and drift); no packageManager / devEngines (either would silently switch on npm caching in setup-node@v6); no cache: input, no npm ci, no ts:check CI step; no Node-version matrix dimension; no dependency or action-version bumps.

Note on the diff size — 7 files, not the 6 you might expect. package-lock.json is included because npm mirrors workspace engines into the lockfile; the diff is exactly the same four engines blocks and nothing else — no dependency change, no version churn, no resolved URL churn. It is a fixed point (a second npm install produces no further change). Committing it keeps the lockfile in sync with the manifests; omitting it would mean every contributor's first npm install immediately dirties their working tree.

Known caveat — the pinned major is EOL. Node 20 "Iron" reached end-of-life on 2026-04-30 (nodejs/Release schedule.json: v20.end = "2026-04-30"), so this pins CI to a runtime that no longer receives security updates. That is deliberate and correctly scoped: this PR makes the repository's existing, implicit support claim (@types/node: ^20.12.7) explicit and testable, without changing it. Moving the project to Node 22 (end: 2027-04-30) or 24 (end: 2028-04-30) is a consumer-visible policy decision that requires bumping @types/node in the same change and absorbing the type fallout; it is tracked as a separate follow-up and must not ride along here. The three values that must move together are engines.node, the two workflow pins, and @types/node — all Node 20 today.

Collision check. Run before writing any code, as required:

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

470 open PRs on the fork. Several are adjacent and pin the CI Node version and/or declare engines.node — notably #133 (.nvmrc 22 + engines >=22.0.0 + node-version-file in both workflows), #509 (node-version literal + root engines >=22.13.0 + a ['22','24'] matrix), and #406/#416/#428/#445/#467/#508/#510/#544/#549. Verified at the git level that none of them has merged: on fork/main there is no .nvmrc, engines is absent from all four manifests, and both workflows still call a bare actions/setup-node@v6 — i.e. the defect described above is still live on main. This PR is branched from current fork/main, not stacked on any of them. They are mutually exclusive by construction — only one Node pin can land — so whichever is reviewed first should be merged and the rest closed. The substantive difference is the value: the siblings pin 22 or 24, this one pins 20 to match the @types/node the repo actually type-checks against, keeping the three values consistent rather than making a support-policy change inside a CI chore. If the maintainers prefer to move the support policy in one step instead, #509 or #133 is the better base and this PR should be closed.

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.

No tests were added, and that is intentional, not an oversight. This change contains zero executable lines — it is four JSON engines blocks and two YAML with: blocks. There is no unit under test and no new-line coverage to report. A test that read package.json and asserted engines.node === '>=20.19.0', or that parsed the workflow YAML, would only restate a constant back to itself: it could never fail for a reason a reader cares about, so it would be noise rather than signal. The real verification for a runtime pin is executing the existing suites on the pinned runtime, which is what the protocol below does, plus the CI matrix itself.

Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, see above: zero executable lines. Existing suites are unmodified.
[x] All unit tests pass locally.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Everything below was run on the pinned runtime. Reproduce with any Node 20 >= 20.19.0:

curl -O https://nodejs.org/dist/v20.20.2/node-v20.20.2-linux-x64.tar.xz
tar -xf node-v20.20.2-linux-x64.tar.xz
export PATH="$PWD/node-v20.20.2-linux-x64/bin:$PATH"

1. Runtime used for every step belownode -v -> v20.20.2 (npm -v -> 10.8.2). Latest Node 20 release, confirmed against https://nodejs.org/dist/index.json.

2. npm install — succeeded. No EBADENGINE warning, which is the live confirmation that the declared >=20.19.0 floor is satisfiable by the tree. git status --short package-lock.json after install shows only the four engines mirrors described above, and a second npm install produces no further change (fixed point).

3. npm run ts:checkfails, and fails identically without this change. Measured both ways on the same Node 20.20.2, with tsc --noEmit --pretty false | grep -c ": error TS":

WITH my change:     281 tsc errors
PRISTINE fork/main: 281 tsc errors      (git stash of all 7 files)
IDENTICAL: yes

ts:check is broken on main independently of this PR (predominantly TS2345/TS2322 in core/test/** and tests/integration/**). This change is JSON and YAML only and cannot affect tsc. ts:check is not a CI gate today and this PR does not add it. Fixing it is out of scope here.

4. npm run build — passes for all three workspaces on Node 20.

5. npm run test:coverage — this is the highest-risk step, since the thresholds were measured on a different Node and V8 coverage can shift across V8 versions. Passes on Node 20 with headroom:

Test Files  224 passed | 20 skipped (244)
     Tests  2678 passed | 44 skipped (2722)
exit code 0

File       | % Stmts | % Branch | % Funcs | % Lines
All files  |   90.64 |    89.43 |   91.39 |   90.64
thresholds |      86 |       87 |      88 |      86

No threshold was touched, and none needed to be.

One environment note, because a naive local run looks alarming: on a workstation with GOOGLE_CLOUD_PROJECT exported, 22 tests/e2e/** suites de-skip (describe.skipIf(!hasAKey) treats that variable as credentials) and then fail with "API key must be provided…", and tests/e2e/live_model_test.ts de-skips because it is gated on describe.skipIf(process.env.CI === 'true'). Neither is related to the Node version. The numbers above are from a run with the CI environment replicated — CI=true, GOOGLE_CLOUD_PROJECT/GEMINI_API_KEY/GOOGLE_GENAI_API_KEY unset — which is what the GitHub runner actually provides.

Did pinning to Node 20 regress anything? Directly measured, rather than assumed. Ran the same suite on Node 20.20.2 and on Node 22.22.2 (this machine's system Node, standing in for the unpinned "before" runtime) and diffed the failure sets:

in node20 but NOT node22 (i.e. regressions caused by the pin):   (empty)
in node22 but NOT node20:  8 suites  -- the known flaky spawned-server
                           integration tests (app_loader, agent_dirname,
                           skills/script_js), which timed out on Node 22
                           and passed on Node 20

Zero failures are unique to Node 20. Node 20 is strictly no worse than the runtime CI has been silently using.

6. npm run lint — passes. npm run format:check — passes ("All matched files use Prettier code style!"). format:check covers only **/*.ts, so the JSON edits are not in its scope, but it gates CI and was run anyway. Separately, prettier --check on the workflow file reports one pre-existing style nit (NODE_OPTIONS: "…" double quotes on line 10) that is present on pristine fork/main too and is untouched here; the lines this PR adds use single quotes, matching Prettier's preference.

7. npm run docs:check — passes (typedoc --emit none --treatWarningsAsErrors).

8. YAML sanity — both workflows re-parsed with a YAML loader to confirm the with: block attached to the correct step and that node-version is a string, not a float:

validation.yaml            run-tests -> {'name': 'Use Node.js', 'uses': 'actions/setup-node@v6', 'with': {'node-version': '20.x'}}
cross-language-integration.yml run-tests -> {'name': 'Use Node.js', 'uses': 'actions/setup-node@v6', 'with': {'node-version': '20.x'}}

No other step in either file was disturbed.

9. Node version observed in CI — "the CI logs show the intended Node version" is an explicit acceptance criterion, so here are the four versions actually reported by the runners on this PR's commit:

Workflow / leg Node reported Before (unpinned, unrelated branch)
validation / run-tests (ubuntu-latest) v20.20.2 v22.23.1
validation / run-tests (windows-latest) v20.20.2 v22.23.1
validation / run-tests (macos-latest) v20.20.2 v24.18.0
Cross-Language Tests / run-tests (macos) v20.20.2

All three OS legs now agree, and the version is chosen by this repository rather than by the runner image. The setup-node step log confirms the resolution path: node-version: 20.x -> Attempting to download 20.x... -> Acquiring 20.20.2.

10. One CI failure, and why it is not caused by this change. The run-tests (windows-latest) leg failed on its first attempt with exactly one test:

FAIL  unit:core core/test/code_executors/unsafe_local_code_executor_test.ts
      > UnsafeLocalCodeExecutor > should execute shell code and return stdout
Error: Test timed out in 5000ms.
 ❯ core/test/code_executors/unsafe_local_code_executor_test.ts:161:3
Test Files  1 failed | 223 passed | 20 skipped (244)

That test shells out to run echo "Hello, Shell!" and hit the 5s default timeout — a process-spawn timing issue on the Windows runner, not a Node API incompatibility. Rather than assume, I checked whether it predates this change: a validation run on the unrelated branch feat/hoist-vertex-placeholder-constants-core-tests, which still uses the bare, unpinned setup-node step and ran on Node v22.23.1, failed with the identical signature — same file, same line 161, same 5000ms timeout, same 1 failed | 223 passed | 20 skipped (244) tally. It is therefore a pre-existing Windows flake, reproducible on a different branch on a different Node major, not a Node 20 regression. Per the error-handling protocol for this change I did not react by loosening engines, moving the pin to a newer major, or editing that test's timeout; fixing the flake is separate work. Re-running that job on the same commit passes: Test Files 224 passed | 20 skipped (244), on node: v20.20.2. All four test jobs (ubuntu / windows / macos / cross-language) are now green.

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. — N/A: the change is declarative config with no code; the rationale is in this description rather than in comments.
[x] I have added tests that prove my fix is effective or that my feature works. — N/A: zero executable lines, see the Testing Plan for why a config-restating test would be noise; the change is proven by running the existing suites on the pinned runtime.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 3, 2026 05:05
Nothing in the repository stated which Node.js runtime it supports: no
engines field in any of the four manifests, no .nvmrc, no .node-version.
The three published packages therefore shipped without an engines field,
so npm could not warn a user installing on an unsupported runtime.

Declare engines.node ">=20.19.0" identically in the root manifest and in
each independently published workspace (@google/adk, @google/adk-devtools,
@google/adk-integrations).

The floor is measured, not guessed: evaluating every engines.node range in
package-lock.json against candidate versions leaves 10 packages unsatisfied
at 20.17.0 and 0 at 20.19.0. The binding constraints are vite@7.3.5
(^20.19.0 || >=22.12.0) and eslint-visitor-keys@5.0.1
(^20.19.0 || ^22.13.0 || >=24). Node 20 is also the major the repository
type-checks against (@types/node ^20.12.7).

A floor rather than an exact pin keeps users on Node 22/24 supported;
engines is a consumer-facing compatibility statement, and with npm's
default engine-strict=false an older runtime warns rather than fails.

package-lock.json carries the same four blocks because npm mirrors
workspace engines into the lockfile; it is a fixed point after the edit.
Both workflows called actions/setup-node with neither node-version nor
node-version-file. That input has no default: with nothing supplied the
action installs nothing and every later run: step executes on whatever
Node the runner image happens to ship. Three consequences, all live: the
tested runtime drifted with the image without any commit here; the
ubuntu/windows/macos legs are independently versioned images and could
run different Node majors in the same matrix; and the executed runtime
was decoupled from the Node 20 API surface the code is type-checked
against.

Pin node-version: '20.x' on both steps. The value is quoted so YAML reads
it as a string, and it pins the major while letting the patch float to the
newest Node 20 (currently 20.20.2), which satisfies the >=20.19.0 floor
declared in the manifests.

node-version-file pointing at package.json was rejected deliberately: it
resolves engines.node, which is the range >=20.19.0, and with the default
check-latest: false the action first looks for a cached semver match, so
the image's newer Node would satisfy it and reproduce the drift this
change removes. engines stays the consumer-facing floor; node-version is
the pin.
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