Ci: pin an explicit Node.js version in CI and declare the supported floor - #509
Open
AmaadMartin wants to merge 2 commits into
Open
Ci: pin an explicit Node.js version in CI and declare the supported floor#509AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 2, 2026 01:54
Both workflows called actions/setup-node with no node-version input, so the tested runtime was whatever Node happened to be baked into the current GitHub runner image -- undeclared, unreviewed, and free to drift when the image is rebuilt. The three-OS matrix did not hold Node constant either, since the ubuntu, windows and macos images are built independently. validation.yaml gains a node-version matrix dimension covering both supported LTS lines (22 and 24), expanding run-tests from 3 legs to 6 and making a Node-version-specific regression attributable to a version rather than an OS. The step is renamed to include the version so it is visible in the job log. cross-language-integration.yml pins a single version: it is a single-OS A2A wire-protocol interop test whose failure modes are serialization and transport, not Node runtime semantics, so one declared version is enough there. This restores a pin that was dropped when setup-node was bumped v3 -> v6 and the node-version: '20' input was deleted in the same hunk. It does not come back as '20', which is end-of-life.
The root manifest declared no engines field, so contributors got no signal about the supported runtime, and README.md promised only "a current Node.js LTS release" -- a claim nothing in the repository asserted or enforced. engines.node is set to >=22.13.0. That floor is derived from the installed dependency graph rather than picked: eslint-visitor-keys@5.0.1 declares ^20.19.0 || ^22.13.0 || >=24, which is the strictest constraint in the tree, and the v20 branch is excluded because Node 20 reached end-of-life. 22.12.0 leaves five packages unsatisfied; 22.13.0 leaves none. There is no .npmrc, so engine-strict is off and this is advisory: an unsupported runtime gets an npm WARN EBADENGINE and the install still succeeds. The README prerequisite now names that same number and the two LTS lines CI actually tests, so the documented floor, the declared floor and the tested versions cannot drift apart silently. This is the dev/CI toolchain floor on the workspace root, which is not a published artifact. Declaring engines on the three published packages is a separate concern and is deliberately not done here.
This was referenced Aug 2, 2026
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
Closes: #issue_number
Related: #issue_number
Problem: Both CI workflows call
actions/setup-node@v6with nonode-versioninput, so the Node.js version under test is whatever the current GitHub runner image happens to ship — undeclared, unreviewed, and free to change when the image is rebuilt.Per the
actions/setup-nodeREADME the input is optional and, when omitted, the action installs no toolchain at all; it just leaves the runner's preinstalled Node onPATH. Three consequences:ubuntu-latest,windows-latestandmacos-latestare independently built images. They agree today, but nothing enforces it, and they skew during staged image rollouts. A matrix leg that differs in two variables cannot attribute a failure to either one.package.jsonhad noenginesfield, andREADME.mdpromised only "a current Node.js LTS release" — a claim no file in the repo asserted.This is a regression rather than an original omission: commit
4365b12bumpedactions/setup-node@v3→@v6and deleted thenode-version: '20'input in the same hunk.Node is also the only unpinned toolchain in these files —
setup-pythonis pinned to'3.11'andsetup-goto'1.25'. This makes the three consistent.Solution: Four edits, 11 insertions and 2 deletions across four files.
.github/workflows/validation.yaml— add anode-version: ['22', '24']matrix dimension, wire it into the step withnode-version: ${{ matrix.node-version }}, and rename the step toUse Node.js ${{ matrix.node-version }}so the version appears in the job log header..github/workflows/cross-language-integration.yml— pinnode-version: '24'. A matrix here was rejected deliberately: it is a single-OS A2A wire-protocol interop smoke test whose failure modes are serialization and transport rather than Node runtime semantics, and it provisions a Go toolchain per job. The Node-version signal comes fromvalidation.yaml.package.json(root) — add"engines": { "node": ">=22.13.0" }.README.md— replace the vague prerequisite with the concrete floor and the tested lines.Why
>=22.13.0, and why both 22 and 24. The floor is derived from the installed dependency graph, not chosen. I scanned all 739 packages innode_modulesthat declareengines.nodeand counted how many each candidate floor leaves unsatisfied:The binding constraint is
eslint-visitor-keys@5.0.1, which declares^20.19.0 || ^22.13.0 || >=24. Its^20.19.0branch is excluded because Node 20 reached end of life on 2026-04-30, leaving 22.13.0 as the lowest version that satisfies the whole tree. Node 22 (maintenance LTS, EOL 2027-04-30) and Node 24 (active LTS, EOL 2028-04-30) are exactly the supported LTS lines today, which is why the matrix tests both: testing only one would make the declared floor an untested claim. Node 26 is deliberately excluded — it is Current, not LTS until 2026-10-28.Because the matrix and
enginesand the README now all name the same numbers, a cross-check asserts they cannot drift apart (see Testing Plan).Scope deliberately held narrow. No
npm ci, nocache: 'npm', nocache-dependency-path, nopackage-manager-cache— all separately tracked. In particularpackageManageranddevEnginesare not added topackage.json: undersetup-node@v6either field set tonpmauto-enables npm dependency caching in every job using the action, which would smuggle the caching change into this diff.enginesis the only new manifest key. No.nvmrc/.node-version/node-version-fileeither — a version file cannot express a two-version matrix, andnode-version-file: 'package.json'would resolve the>=22.13.0range to the newest satisfying release, which today is Node 26 (non-LTS). Workspace manifests andpackage-lock.jsonare untouched; declaringengineson the three published packages is a separate concern.Collision check (required, and it found a lot). Before writing anything I ran
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000(409 open PRs) and diffed every plausibly adjacent one. Seven open PRs touch these same twosetup-nodesteps: #133 and #508 (both.nvmrc+node-version-file, plusengineson the workspace manifests), #428 ('24'in both workflows), #416 (.nvmrcin both), #406 ('22'in both), #467 ('24'invalidation.yaml, bundled withnpm ci), plus #443 (README floor only) and #445 (engineson published packages only).I did not stack on any of them: they mutually conflict on the same lines, so at most one can land, and the two closest (#133, #508) use the
.nvmrc+node-version-filemechanism this change deliberately avoids for the reason above. Note also that #508 declares>=20, which is EOL, and #133 declares>=22.0.0, which the table above shows leaves six packages unsatisfied. Maintainers should expect to close the duplicates in favour of whichever approach they prefer.Two operational notes for the merger:
run-testscurrently has noname:, so its checks arerun-tests (ubuntu-latest)etc. Adding the matrix dimension renames them torun-tests (<os>, <node>). If branch protection onmainlists the old names as required checks, an admin must update them or PRs will block on checks that no longer exist. This cannot be fixed from a branch.fail-fastis left at its default, so one red leg cancels its siblings — re-run to see the full picture.One honest gap:
>=22.13.0as a range also admits Node 23.x, whicheslint-visitor-keysexcludes. In practice this is academic — 23 is an odd-numbered Current line that is already EOL and no one should be running it — and expressing it exactly (^22.13.0 || >=24) would state a range where the task asked for a floor.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.
No unit test file was added, and that is deliberate — it must not be faked. This change adds zero lines of executable TypeScript; it is three declarative config lines plus one manifest key. There is no new code to cover and no test that could meaningfully assert on it at runtime, so adding a test file purely to manufacture a coverage number would be dishonest. The equivalent rigour is the mechanical assertions below, run against the committed tree.
A1 parses both workflows with
js-yaml(already in the tree; no dependency added) and asserts each has exactly onesetup-nodestep carrying a non-emptynode-version. This is the assertion that catches the silent failure mode — a typo'd expression yields an empty input and falls back to system Node, i.e. the exact bug being fixed, with no YAML parse error to warn you. A5 re-derives the floor fromengines.nodeand the tested lines from the matrix and greps both out ofREADME.md, so the three can never drift.Proof the assertions can fail. Each was run against the unfixed tree (
git stashof all four files), and each failed with a specific message:Error: .github/workflows/validation.yaml: setup-node has no node-version inputError: expected 6 matrix legs, got 0Error: bad engines: undefinedError: no engines.node to compare README againstTwo further targeted line mutations against the fixed tree, to prove the assertions pin the exact values rather than merely their presence:
node-version: ['22', '24']→[22, 24](unquoted)Error: node-version values must be quoted stringsengines.node→">=22.0.0"Error: floor 22.0.0 does not satisfy eslint-visitor-keys enginesandError: README floor does not match engines.node (22.0.0)The quoting matters: unquoted integers are harmless today but become a footgun the moment someone writes
22.13, which YAML parses as a float and truncates.Guards run on the committed tree (unchanged by this diff, run to prove no collateral damage):
npm run lintclean,npm run format:checkclean,npx prettier --check package.json README.mdclean,git diff fork/main --name-onlylists exactly the four intended files, andpackage-lock.jsonis byte-identical after a fullnpm install.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
This PR is its own end-to-end test — both edited workflows trigger on
pull_request: branches: [main], so opening it exercises the change for real. On the Checks tab, confirm:run-tests (<os>, <node>)checks appear instead of three.Cross-Language Tests / run-testsis green and its "Use Node.js" log shows a 24.x version.EBADENGINEwarning in any leg'snpm install(both 22 and 24 satisfy>=22.13.0).To verify the
enginesdeclaration is advisory and cannot break a contributor:There is no
.npmrcin the repo, soengine-strictis off;engineswarns and never fails an install.If a Node 24 leg fails, that is a real finding — Node 24 has never been exercised by this repo. It is reported here rather than masked; the pin is not silently downgraded to hide it.
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.
CI results on this PR (the end-to-end test above, actually run)
All 9 checks pass. The six matrix legs materialized exactly as designed:
Each leg's step header renders the interpolated version (
Use Node.js 22/Use Node.js 24), and the action now resolves a real toolchain instead offalling back to the runner's system Node:
22.23.1 is the version the runner images preinstall, so the
'22'legpreserves exactly what CI was already doing by accident; 24.18.0 is the
net-new coverage this PR buys. No
EBADENGINEwarning appeared in any leg.One flake, reported rather than hidden. On the first run,
run-tests (windows-latest, 24)failed — 2679 tests passed, one test filedid not:
I re-ran that single job against the same commit with no code change and it
passed (7m35s), which makes the failure non-deterministic rather than a Node 24
incompatibility. The test spawns the ADK CLI as a child process and waits for a
startup message, which is the same shape as the Windows/macOS child-process
timing flakes already tracked elsewhere in this repo; it touches no code this PR
changes (this PR changes no TypeScript at all). I have not downgraded the
Windows pin to
'22'to make it disappear — the pin stands, and the flake isfiled as separate work.
Worth noting for reviewers: this is arguably the matrix earning its keep on day
one. A pre-existing Windows flake that was previously a one-in-three-legs event
is now a one-in-six-legs event, so
fail-fastwill trip more often until theunderlying test is stabilized.