Skip to content

Chore: declare a supported Node.js range (>=20.19.0) and exercise the floor in CI - #571

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

Chore: declare a supported Node.js range (>=20.19.0) and exercise the floor in CI#571
AmaadMartin wants to merge 2 commits into
mainfrom
feat/engines-node-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):
    Related: Docs: document minimum supported Node.js version in README google/adk-js#526

  2. Or, if no issue exists, describe the change:

Problem: The published packages carry no machine-readable statement of the Node.js runtime they support. npm cannot warn a consumer who installs @google/adk on an unsupported Node, and CI cannot catch a change that silently requires a newer runtime than the project claims to support, because both workflows call actions/setup-node@v6 with no with: block and therefore run on whatever Node the GitHub-hosted runner image happens to ship that week. The README prerequisite added in google#526 ("ADK for TypeScript requires a current Node.js LTS release") has nothing machine-checkable behind it. adk-python pairs the same prose with an enforceable requires-python = ">=3.10".

Solution: Three coordinated changes.

  1. "engines": {"node": ">=20.19.0"} on the three published manifests (core, dev, integrations), mirrored into the lockfile's workspace entries.
  2. Both workflows get an explicit Node version. validation.yaml becomes an explicit include: matrix — the three OSes on the current line, plus one extra job pinned to the declared floor, so the floor is executed rather than merely asserted.
  3. A drift-guard test that fails if the three manifests ever stop agreeing.
os node
ubuntu-latest 24.x
windows-latest 24.x
macos-latest 24.x
ubuntu-latest 20.19.0 (declared floor)

Verified on this PR: those four jobs are exactly what CI produced, and the floor job's log shows setup-node acquiring v20.19.0 and running the full test:coverage (225 test files passed), lint, format:check and docs:check -- not a skipped or no-op job.

The matrix is a base (os x ['24.x']) plus a single include entry for the floor. Per the workflow syntax docs, an include object whose keys would overwrite an original matrix value "cannot be added to any of the matrix combinations", so "a new matrix combination will be created instead" -- 3 + 1 = 4 jobs, not 6. This keeps the existing os: line untouched.

cross-language-integration.yml has no matrix and does not exercise the floor; it is pinned for the same reason the other workflow is -- so that no workflow silently inherits whatever Node the runner image ships that week. Without it the repository would still have one workflow whose Node version drifts with the runner image.

Root package.json deliberately gets no engines: npm already validates each workspace package's engines during a root install, so a root block is never published and is just a fourth copy of the number to drift.

Why >=20.19.0

This is the one judgement call here, so it is measured from package-lock.json rather than chosen. Walking every lockfile entry that declares engines.node (workspace roots excluded, which would be circular):

Closure Min Node on the 20 line Min Node on the 22 line Binding constraint
Production only (dev: true excluded, 426 entries) 20.6.0 22.0.0 @opentelemetry/core ^18.19.0 || >=20.6.0
Full lockfile incl. dev toolchain (797 entries) 20.19.0 22.13.0 vite (via vitest) ^20.19.0 || >=22.12.0; eslint-visitor-keys ^20.19.0 || ^22.13.0 || >=24

>=20.19.0 is the single value simultaneously true for consumers and contributors: at or above the runtime floor, and exactly the toolchain floor on the 20 line. Nothing in either closure requires anything above 20. Verified empirically — npm ci on Node 20.19.0 produces zero EBADENGINE warnings.

Supporting reasons: @types/node is ^20.12.7 in the root and dev manifests, so the codebase is type-checked against the Node 20 stdlib and a higher floor would be incoherent without also bumping it (a separate change); and @google/genai@2.9.0, which @google/adk is built on, declares >=20.0.0 — a library should not advertise a stricter floor than the SDK it wraps.

Why not >=22. Node 20 is past end-of-life, which is a real argument for a 22 floor, but engines is a compatibility floor ("will this break?"), not a lifecycle recommendation ("what should you run?"); the README already carries the recommendation, and the two are complementary. A 22 floor would emit EBADENGINE at users who work fine today and contradict @types/node@^20. Note also that >=22.12.0 specifically is not the floor of either closure — production is satisfied at 22.0.0 and the full lockfile not until 22.13.0, so 22.12.0 (vite's dev-only bound) leaves 5 EBADENGINE warnings from eslint-visitor-keys on the floor job itself. If maintainers do want 22, it is a one-value edit in three manifests plus the matrix entry, and it should ride with the @types/node bump.

Collision check

Checked before writing anything (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000, 470 open PRs). This area is contested: #445, #508 and #544/#549 each implement the manifest half independently, and #509/#133/#510/#406/#416/#428/#467 overlap the CI-pin portion. They disagree on the floor (>=22.12.0, >=20, 22.13.0), and none of them both declares a floor and runs it. This PR uses the measured value and is self-contained; it is intended to supersede those, and they should be closed rather than merged alongside it.

Consequences

  • npm WARN EBADENGINE for consumers below 20.19.0, from the first release containing this change. Installs still succeed — there is no .npmrc with engine-strict, so this never hard-fails. Users on 20.6–20.18 will see a warning while continuing to work; that is the deliberate cost of a single number.
  • Matrix job display names change, from run-tests (ubuntu-latest) to run-tests (ubuntu-latest, 24.x). If branch-protection required checks are pinned to the old names, a maintainer must update them.
  • CI cost rises by one ubuntu job per push/PR.
  • README.md:55 still reads "requires a current Node.js LTS release". That prose is deliberately left alone (it landed in Docs: document minimum supported Node.js version in README google/adk-js#526, and restating a number there re-litigates that review), but note it is now a vaguer fourth statement of the same fact and reads as slightly at odds with a maintenance-LTS floor. Tightening it is worth a follow-up.
  • Release tooling is unaffected; engines is not a field release-please manages. Suggested squash title is the commit subject (chore:). Because the EBADENGINE warning is user-visible, retitle to fix: if you want a changelog line and a patch release — that is a maintainer call, not made 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.

tests/integration/package_manifests/engines_test.ts asserts the three published manifests declare engines.node and agree. It deliberately does not pin the literal '>=20.19.0': that would make the test a fourth copy of the number and force a test edit on every deliberate bump, whereas mutual agreement is the property that actually breaks by accident. Asserting on the whole object (rather than looping) makes the vitest diff name the offending workspace.

This change adds no production TypeScript. Coverage include in vitest.config.ts is {core,dev,integrations}/src/**/*.ts, so the new file is outside the denominator and cannot move the existing thresholds (statements: 86, branches: 87, functions: 88, lines: 86). The coverage requirement is satisfied vacuously; the real regression signal is the guard test plus the CI floor job.

[x] All unit tests pass locally.

Prove the test can fail — three mutations, each run against the committed tree, then reverted:

# Mutation Result
1 dev/package.json">=22.0.0" FAILS, and the diff names the workspace: - "dev": ">=20.19.0" / + "dev": ">=22.0.0"
2 delete the engines block from integrations/package.json FAILS: - "integrations": ">=20.19.0" / + "integrations": undefined
3 delete the engines block from core/package.json (the reference entry) FAILS: AssertionError: expected undefined to be defined at expect(reference).toBeDefined()

Unmutated: Test Files 1 passed (1) / Tests 1 passed (1).

Everything below was run on Node 20.19.0 — the exact runtime the new floor job pins (downloaded from nodejs.org/dist/v20.19.0/), with GOOGLE_CLOUD_* unset to match a clean runner:

$ npm ci                 -> 0 EBADENGINE warnings
$ npm run build          -> OK
$ npm run lint           -> OK
$ npm run format:check   -> OK
$ npm run docs:check     -> OK
$ npx vitest run --project unit:core --project unit:dev
   Test Files  182 passed (182)
        Tests  2574 passed (2574)
$ npx vitest run --project integration tests/integration/package_manifests/engines_test.ts
   Test Files  1 passed (1)   Tests  1 passed (1)
$ bash scripts/check_license.sh  -> All files have the correct license header.

Note: with an ambient GOOGLE_CLOUD_PROJECT exported, dev/test/cli/cli_create_test.ts > should handle Vertex AI selection with gcloud defaults fails. That is a pre-existing env-hermeticity bug unrelated to this diff — it reproduces identically on Node 22 and is the subject of separate open PRs. It passes on every runtime once the variable is unset.

Manual End-to-End (E2E) Tests:

  1. The lockfile diff is exactly what the change implies. npm install --package-lock-only && git diff --stat package-lock.json+9 −0, three lines per workspace entry, nothing else. Re-checked after committing to confirm prettier did not reformat it.

  2. The warning actually reaches a consumer below the floor. On Node 18.20.8, npm install at the repo root:

    npm warn EBADENGINE   package: '@google/adk@1.5.0',
    npm warn EBADENGINE   required: { node: '>=20.19.0' },
    npm warn EBADENGINE   package: '@google/adk-devtools@1.5.0',
    npm warn EBADENGINE   required: { node: '>=20.19.0' },
    npm warn EBADENGINE   package: '@google/adk-integrations@1.5.0',
    npm warn EBADENGINE   required: { node: '>=20.19.0' },
    

    All three published packages are named, and the install still succeeds (exit=0) — confirming the advisory-only behaviour.

  3. The field actually ships. npm pack --workspace @google/adk, then reading package/package.json out of the tarball → engines: {'node': '>=20.19.0'}. It reaches consumers rather than stopping at the repo.

  4. The floor job is not vacuous. Confirm on this PR that the job list contains run-tests (ubuntu-latest, 20.19.0) and that it ran the full build and test suite. The pin is to the exact patch rather than 20.x so that a Node API added later in the line cannot pass CI while breaking a consumer who is still inside the declared range.

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.

The published packages carried no machine-readable statement of the Node.js
runtime they support, so npm could not warn a consumer installing on an
unsupported runtime, and CI ran on whatever Node the GitHub-hosted runner
image happened to ship that week.

Declare engines.node ">=20.19.0" on the three published manifests and mirror
it into the lockfile's workspace entries. The value is the single number that
is true for both audiences, measured from package-lock.json: the production
dependency closure is satisfied from 20.6.0 (@opentelemetry/core), and the
full closure including the dev toolchain is satisfied from exactly 20.19.0
(vite via vitest, eslint-visitor-keys). No entry in either closure requires
anything above 20.

Give both workflows an explicit Node version so the runner default stops
deciding, and add a fourth validation job pinned to the declared floor so the
floor is actually executed rather than merely asserted.

A drift-guard test fails if the three manifests ever stop agreeing. It checks
presence and mutual agreement rather than the literal range, so a deliberate
bump does not require editing the test.
@AmaadMartin
AmaadMartin force-pushed the feat/engines-node-floor-ci-matrix branch from a94dbe3 to 3a877ad Compare August 3, 2026 12:19
@AmaadMartin AmaadMartin changed the title Ci: pin Node.js in both workflows and exercise the declared engines floor (stacked on #445) Chore: declare a supported Node.js range (>=20.19.0) and exercise the floor in CI Aug 3, 2026
@AmaadMartin
AmaadMartin changed the base branch from feat/declare-engines-node-range to main August 3, 2026 12:19
An include entry that overwrites an original matrix value cannot be merged
into any existing combination, so it creates a new one: a base of
os x ['24.x'] plus the single floor entry still yields four jobs, without
hand-writing the three OS rows. Leaves the os: line untouched.
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