Chore(ci): pin Node.js 24 and enable npm caching in both CI workflows - #428
Open
AmaadMartin wants to merge 2 commits into
Open
Chore(ci): pin Node.js 24 and enable npm caching in both CI workflows#428AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
July 31, 2026 18:37
Both setup-node steps ran with no inputs, so each CI leg used whatever Node.js the runner image happened to ship and npm caching was off. The validation workflow fans out over ubuntu/windows/macos, so a single run could execute on three different Node majors, and differing npm majors normalize package-lock.json differently. Pin node-version: '24' and set cache: 'npm' in both workflows, matching how python-version and go-version are already pinned in these same files. Node 24 'Krypton' is the current Active LTS (since 2025-10-28); Node 20 reached EOL on 2026-04-30. Declare the same range as engines.node in the root package.json (which is unpublished, so no consumer install is affected) and mirror it into the lockfile root entry by hand. Note the prerequisite in CONTRIBUTING.
… note engines.node is not load-bearing for the pin: setup-node is fed the literal node-version: '24', not node-version-file, so nothing in CI reads the field. Its only effect was an EBADENGINE warning on local installs. It was also half-applied and internally inconsistent. Root would have been the only manifest declaring it, so package consumers -- the people a runtime floor is for -- got no signal, while @types/node stays at ^20.12.7 in both the root and dev manifests, type-checking against the Node 20 stdlib surface. Declaring a support floor is a policy decision that belongs on its own change, alongside the workspace manifests and a matching @types/node bump. Dropping it also leaves package-lock.json untouched, so the setup-node cache key stops churning: the key is hashFiles(package-lock.json) with no restoreKeys, so editing the lockfile forced a cold miss on every OS. The CONTRIBUTING note loses its pointer to engines.node, which would have become a dangling reference, and keeps the part contributors need.
This was referenced Aug 1, 2026
Ci: pin Node 24, install with npm ci, and fail validation when package-lock.json is out of date
#467
Open
Open
Open
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 GitHub Actions workflows call
actions/setup-node@v6with no inputs at all —.github/workflows/validation.yaml:23-24and.github/workflows/cross-language-integration.yml:17-18. Two consequences:node-version, the action installs nothing and the job uses whatever Node.js/npm the runner image happens to ship.validation.yamlfans out overubuntu-latest,windows-latestandmacos-latest, so one CI run can execute on three different Node majors, and those majors change silently whenever GitHub refreshes an image. Since different npm majors normalizepackage-lock.jsondifferently, an unpinned toolchain is itself a source of lockfile churn.setup-node@v6auto-enables caching only when the rootpackage.jsondeclares npm via a top-levelpackageManagerordevEngines.packageManagerfield (thepackage-manager-cacheinput). This repo declares neither and passes nocache:input, so caching is off and every job on every OS re-downloads the full tree (~1090 packages).This is also internally inconsistent: every other toolchain in these same files is pinned —
setup-python@v5withpython-version: '3.11',setup-go@v5withgo-version: '1.25'. Node.js is the only unpinned one.Solution: Pin
node-version: '24'and setcache: 'npm'on bothsetup-nodesteps, matching the single-quote style of the neighbouringpython-version/go-versioninputs, plus a one-line contributor note. Three files, 8 added lines, nothing removed.Why 24, grounded in
nodejs/Releaseschedule.jsonrather than memory:README.mdsays ADK for TypeScript "requires a current Node.js LTS release"; the current Active LTS is 24. Major-only ('24', not'24.10.0') fixes the actual defect — differing Node/npm majors across legs — while still picking up security patches and preferring a tool-cache hit (check-lateststays at its defaultfalse).Deliberately not in this PR:
engines.node. An earlier revision addedengines.node: ">=24"to the rootpackage.jsonand mirrored it intopackage-lock.json. I removed it. Nothing in CI reads it —setup-nodeis fed the literalnode-version, notnode-version-file— so it was not load-bearing for the pin; its only effect was anEBADENGINEwarning on local installs. It was also half-applied and self-contradictory: root would have been the only manifest declaring it, so package consumers got no signal, while@types/nodestays at^20.12.7in both the root anddevmanifests, i.e. the repo type-checks against the Node 20 stdlib surface. Declaring a support floor is a policy decision that deserves its own PR, moving the workspace manifests and@types/nodewith it. Dropping it also leavespackage-lock.jsonuntouched, so this PR carries no lockfile churn at all..nvmrc/.node-version/packageManager, no new files. Inlinenode-versionkeeps each workflow self-describing and matches howpython-version/go-versionare already handled in these same files. ApackageManagerfield would auto-enable caching but activates Corepack shims repo-wide — a far larger behavioural change than a download cache.checkout@v6,setup-node@v6,setup-python@v5,setup-go@v5unchanged), no step reordering/renaming, andnpm installstaysnpm install(switching tonpm ciis a separate concern).README.mduntouched. Line 55 is the consumer prerequisite for installing@google/adk; narrowing it to "24+" would silently change the library's stated support policy. Contributor guidance goes inCONTRIBUTING.mdinstead.node-version-file: 'package.json'driving CI offengines.node—setup-noderesolves anenginesrange to the newest satisfying release, so">=24"would select the non-LTS line the moment 25.x exists, trading one form of non-determinism for another. And a Node-version matrix (22 and 24), which doubles CI cost; the goal is determinism, not broader coverage.Collision check (required, and it found real overlap). Before writing anything I ran
gh pr list --repo AmaadMartin/adk-js --state open --limit 100and diffed every plausibly adjacent PR. This area is heavily contested — four open PRs plus one unPR'd branch already attack it:chore/ci-npm-cache-setup-nodenode-version: '22'+cache: 'npm', both workflowsfeat/ci-pin-node-and-npm-cache.nvmrc(22) +node-version-file+cache, both workflows; its 2nd commit81802bf2also drops a rootengines.nodeblockfix/cross-language-workflow-npm-cachecache: npmon cross-language only, plus an unrelatedRoutedAgent.clonechangefix/ci-npm-cache-node-pincache: 'npm'on validation, stacked onfeat/node-version-source-of-truthfeat/node-version-source-of-truth(no PR).nvmrc22 +engines.node >=22.0.0across all workspace manifestsThese siblings contradict each other, and all five pin 22 (Maintenance) rather than 24 (Active LTS). I did not stack on any of them: #416/#296 build on
.nvmrc+node-version-file, a mechanism this change deliberately avoids, and #416 removes the same rootenginesblock I also concluded should go — two independent attempts reaching the same call, which is decent evidence it does not belong in a CI pin. Only one of these should land. If a reviewer prefers a sibling's approach, close this one; if this lands, the others need closing or rebasing. Flagging rather than silently adding a sixth variant.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 tests were added, deliberately. This diff is two GitHub Actions
with:blocks and one Markdown line — it adds zero lines of executable TypeScript/JavaScript. There is no coverage target and nothing a mutation could be injected into, so the usual "prove the test can fail" step has no subject. A test asserting the literal contents of a YAML file would be a tautology that restates the file it reads, so I did not manufacture one. I confirmed no existing test reads.github/workflows/**(grep -rlovercore/test,dev/test,testsreturns nothing), so no existing test needed updating either, and coverage cannot move because no source file is touched.Verification is static and toolchain-level instead. On the final commit:
1. The edited YAML parses to exactly the intended step (via
js-yaml, already a workspace dep):Both
setup-nodecall sites in the repo are covered — these are the only two, and no other workflow invokes node or npm.2. Local check suite — every step CI runs:
npx secretlint "**/*"npm run buildnpm run lintnpm run format:checknpm run docs:checknpx prettier --check CONTRIBUTING.mdnpx vitest run --project unit:core core/test/utils/case_utils_test.ts core/test/utils/variant_utils_test.tsThe
prettier --checkon the Markdown is becausenpm run format:checkonly covers**/*.tswhile husky's lint-staged formats{json,md}at commit time — so the hook cannot silently reformat it. The targeted vitest run is a harness sanity check, not a test of this change.npm run test:coverageruns the whole repo suite; I did not run it locally (my instructions prohibit whole-suite runs and it exercises only untouched code) — it is one of the six steps CI runs on all three OS legs, which is the authoritative gate.Local Node version. This environment is Node v22.22.2 / npm 9.2.0, and no Node 24 toolchain is available (no nvm/fnm/volta), so every local check above ran on Node 22. If a step depends on runtime behaviour that differs on 24, this PR's CI run is what surfaces it; I am not implying otherwise.
3. CI: all four test legs green on the first attempt —
run-tests (ubuntu-latest)4m49s,(macos-latest)6m35s,(windows-latest)8m2s,Cross-Language Tests1m21s, pluscheck-license. Every leg loggednode: v24.18.0, i.e. the pin resolved to the same major on all four, which is the determinism this change exists to create.Two things I got wrong while validating this, corrected here because they change how you should read the caching benefit.
When the cache actually starts paying off: after this merges, not on this PR. Every leg of the final run still logged
npm cache is not found. GitHub scopes Actions caches by ref: a run can restore from its own ref or from the default branch, but not from a sibling PR's scope. Every entry on this repo today sits under somerefs/pull/<n>/merge— there are none onrefs/heads/main, becausemain's workflow has never hadcache: 'npm'to save one. So the first run of every PR is a cold miss, this one included, and it stays that way until this change lands onmainand one post-merge run populates the three per-OS entries. From then on PR branches inherit them and get a real first-run hit. Caching is nonetheless demonstrably wired up: thePost Use Node.jsstep saved 85 MB (macOS arm64) and 86 MB (Linux x64), and a re-run within this PR's own scope restored one (Cache hit for: node-cache-macOS-arm64-npm-8ef87f25…). Before this change there was nopostcache step at all.I had attributed the siblings' clean runs to their leaving
package-lock.jsonalone. That was wrong — ref scoping, not lockfile content, is what decides a first-run miss, and they were cold on their first runs too.Two integration tests are timing-marginal under a cold cache. An earlier revision of this PR also edited
package-lock.json, and on those runs macOS and Windows failed:tests/integration/app_loader/app_loader_test.ts(and on Windows alsotests/integration/adk_web/webui_test.ts) withTest timed out in 40000ms, vitest reportingcollect 236.47s. Both tests shell out to a realnpm installinside fixture directories inbeforeAll, sharing the same 40sTEST_EXECUTION_TIMEOUTas their assertions, so a cold download cache can eat the entire budget and a slow network gets reported as a logic failure.This was never a Node 24 incompatibility, and the evidence is unusually clean: the same commit on the same
node: v24.18.0both failed and passed. macOS failed cold and passed warm; Windows failed cold and then passed on a second still-cold attempt; and the final commit above passed on all four legs while cold. A runtime incompatibility does not come and go between re-runs. Every failure was a timeout — never an assertion, never a runtime error — and 2677–2680 tests passed in every run including the failing ones.So the flakiness is real but pre-existing and independent of this change: any cold cache can trip it, which today means the first run of any PR, and after this merges will mean the first run after any dependency bump. I deliberately did not skip, weaken, or retime those tests — greening my own CI by editing an unrelated test destroys the signal, and raising the magic number treats the symptom. It is filed as separate follow-up work to remove the per-fixture network install, or failing that to give it a timeout distinct from the assertion timeout.
One incidental observation for whoever picks that up: Windows derives a different digest than Linux/macOS for the same lockfile (
701b40c3…vs413b02d6…), almost certainly line-ending conversion at checkout. It is stable per OS —701b40c3…recurs across runs — so this is not instability, just a separate Windows cache namespace that warms on its own.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
The real end-to-end test is this PR's CI run; it cannot be reproduced locally. What to check in the Actions logs:
validationleg (ubuntu-latest,windows-latest,macos-latest) and onCross-Language Tests. Each should report resolving a 24.x — the same major on all four, where previously the step installed nothing and each leg silently inherited the image default.Post Use Node.jsstep. Its presence is the proof caching is wired up; before this change nopostcache step existed at all. Cache entries observed during development: 85 MB (macOS arm64), 86 MB (Linux x64).secretlint,build,test:coverage,lint,format:check,docs:check), and thatLicense Header Checkis untouched (scripts/check_license.shonly scans*.js/*.ts, and this PR adds no new files).To reproduce the toolchain locally:
nvm install 24 && nvm use 24 && npm install && npm run build && npm run test:coverage.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.
On the two boxes that cannot literally be true here: no tests were added because the change contains no executable code (reasoning above), and there is no code to comment — the rationale that would otherwise be a comment is in this description, keeping the YAML free of narration.