Ci: pin Node 24, install with npm ci, and fail validation when package-lock.json is out of date - #467
Open
AmaadMartin wants to merge 2 commits into
Open
Ci: pin Node 24, install with npm ci, and fail validation when package-lock.json is out of date#467AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 1, 2026 10:26
`actions/setup-node@v6` was invoked with no inputs, so the job installed nothing and ran on whatever Node -- and therefore whatever bundled npm -- the runner image happened to ship. Pin `node-version: '24'`: Node 24 (Krypton) has been Active LTS since 2025-10-28 and is supported until 2028-04-30, while Node 20 (Iron) reached end-of-life on 2026-04-30. The value is quoted because YAML parses an unquoted two-segment version such as 20.10 as a float. Install with `npm ci` rather than `npm install`. `npm ci` installs strictly from the lockfile and hard-errors when the manifests and the lock disagree, instead of silently repairing the lockfile in the runner's throwaway workspace and discarding the repair.
Nothing enforced that the committed package-lock.json is what npm resolves from the committed package.json, so lockfile rot never surfaced in the pull request that caused it. It accumulated and landed as collateral churn elsewhere: the stale `"dev": true` flag on adm-zip@0.5.17 was finally removed inside google#564, a change about hoisting @google/genai, as 142 lines of lockfile diff a contributor then had to justify by hand. Regenerate the lockfile after the install and fail with an actionable annotation when the result differs from what is committed. The regeneration is load-bearing: npm ci never writes a package-lock, so `npm ci` followed by a bare `git diff --exit-code` is a check that can never fail -- worse than adding nothing, because it looks like protection. The gate is also not redundant with npm ci, which only verifies that the manifests' dependency specs are satisfied by the lock; resolution metadata like the adm-zip flag is recomputed only by regeneration. The `if ! git diff ...; then ... fi` wrapper is required because a multi-line run block executes under `bash -e`, which would abort the step before the echo could emit the annotation. Both outputs are kept: the diff says what drifted, the annotation says what to do about it. Lockfile content does not vary by platform under lockfileVersion 3, so the gate runs once on ubuntu-latest; the repo has no .gitattributes, so running the diff on windows-latest would risk whole-file noise from CRLF conversion.
AmaadMartin
changed the base branch from
fix/ci-pin-node-version-npm-cache
to
main
August 1, 2026 17:30
AmaadMartin
force-pushed
the
feat/ci-lockfile-drift-check
branch
from
August 1, 2026 17:30
71c4dcd to
ebcd44b
Compare
This was referenced Aug 2, 2026
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
N/A — no existing issue.
Problem: Nothing enforces that the committed
package-lock.jsonis what npmactually resolves from the committed
package.json. Thevalidationworkflowinstalls with
npm install, which silently repairs a stale lockfile in therunner's throwaway workspace and then discards the repair. Lockfile rot
therefore never surfaces in the PR that caused it; it accumulates and lands as
collateral churn in somebody else's PR. Concrete precedent: the
package-lock.jsonentry foradm-zip@0.5.17carried a stale"dev": trueflag long after
adm-ziphad become a production dependency. It was finallyremoved as collateral in
00f37755, "fix(deps): hoist @google/genai 2.x to theworkspace root" (#564) — 142 lines of lockfile churn in a PR about something
else, which a contributor then had to justify by hand.
The install was also not reproducible:
actions/setup-node@v6ran with noinputs at all, so the job installed nothing and used whatever Node — and
therefore whatever bundled npm — the runner image happened to ship.
Solution: three edits to
.github/workflows/validation.yaml, and nothingelse. The PR changes exactly one file.
node-version: '24'. Node 24 (Krypton) has beenActive LTS since 2025-10-28 and is supported until 2028-04-30; Node 20
(
Iron) reached end-of-life on 2026-04-30 (verified againstnodejs/Releaseschedule.json). Quoted deliberately — YAML parses an unquoted two-segmentversion such as
20.10as the float20.1.npm ciinstead ofnpm install, so the committed lockfileis authoritative rather than something the runner quietly repairs.
Check package-lock.json is up to datestep immediately after theinstall: regenerate the lockfile and fail with an actionable annotation if
the result differs from what is committed.
The pin and the gate are a package deal and ship together on purpose: without
the pin the gate would be non-deterministic across runner-image updates and
would eventually red-line every PR for no reason.
Three design points a reviewer should not have to re-derive:
npm cinever writes a package-lock (npm docs: "It will never write to package.json
or any of the package-locks: installs are essentially frozen"), so
npm cifollowed by a bare
git diff --exit-codeis a check that can never fail —worse than adding nothing, because it looks like protection. The step runs
npm install --package-lock-onlyfirst. The mutation test below proves thisempirically rather than by argument.
npm ci.npm cionly errors when thelock cannot satisfy the manifests' dependency specs. It exits 0 on both
drift classes tested below — including the motivating one, a stale
"dev": true, which is resolution metadata that only regenerationrecomputes.
ubuntu-latest. Lockfile content does not vary byplatform under
lockfileVersion: 3, and the repo has no.gitattributes, sorunning the diff on
windows-latestwould risk whole-file noise from CRLFconversion.
The
if ! git diff …; then … fiwrapper is required rather than a baregit diff --exit-code: multi-linerun:blocks execute underbash -e, whichwould abort the step before the
echocould emit the annotation, losing theremediation message. Both outputs are kept — the diff says what drifted, the
annotation says what to do. The diff is scoped to
package-lock.jsonso anunrelated file touched by an earlier step cannot trip it.
Collision check. Per the pre-work check
(
gh pr list --repo AmaadMartin/adk-js --state open --limit 100, thengh pr diff <n> --name-onlyon every plausibly adjacent PR), six open PRs touchCI install/toolchain configuration:
validation.yaml('24'/.nvmrc22 /'22'), and all three additionally add an npm cache. They compete with each other as much as with this PR.npm install→npm ciinvalidation.yaml.cross-language-integration.ymlonly — deliberately untouched here.deps:checkstep at the end of the same job; no conflict.No open PR adds a lockfile drift gate, which is the substance of this
change, so this is not a duplicate of any of them. The pin and the
npm ciline do overlap #428/#416/#406 and #415 respectively. This PR was initially
stacked on #428's branch to avoid restating the pin, but that base also carries
edits to
.github/workflows/cross-language-integration.ymlandCONTRIBUTING.md, and thecross-language-integration.ymlchange would collideline-for-line with the separately queued task for that file. Keeping this PR to
a single file was worth more than avoiding the three-line overlap, so it is
based on
mainand carries the pin itself. If any of those PRs lands first theoverlapping hunks are identical or near-identical one-liners and rebase
trivially; only the pin's spelling could change, and the gate is unaffected.
Not in scope:
.github/workflows/cross-language-integration.ymlalso runs abare
npm installunder an unpinnedsetup-node. It is queued separately andis deliberately not edited here so the two changes cannot conflict. No npm cache
is added either — that is unrequested here and is what #428/#416/#406 are for.
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:
[ ] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Unit tests are not applicable: this change ships no executable code — it is
a GitHub Actions workflow document. There is nothing to import and assert on, so
no test file was added and
vitest.config.tsis untouched. Coverage isdischarged instead by exercising both branches of the new gate locally, plus
a mutation test. Every run below executes the shipped script, extracted
straight out of the YAML and run under
bash -eexactly as GitHub runs amulti-line
run:block:All runs are on Node 24.18.1 / npm 11.16.0 — the exact pair the pin resolves
to (
nodejs.org/dist/index.json).1. Positive control — the gate passes on a clean tree.
The committed 15,680-line lockfile round-trips byte-identically under the pinned
toolchain, so the gate is green on
maintoday. (This was the precondition tocheck before writing any YAML: had it reported drift, either a regenerated
lockfile or a
'22'pin would have had to ship with this PR. Neither is needed.)2. Negative control — the gate fails on real drift. Drift introduced the way
a contributor introduces it: edit a manifest, do not regenerate the lock.
Worth noting:
npm ciexits 0 here, because the locked4.0.9still satisfies^4.0.8. The install step alone would not have caught this. Both the diff andthe annotation are present, which is the check that the
if ! …wrapper works —a bare
git diff --exit-codeunderbash -ewould have exited before theannotation was emitted. Manifest and lockfile were restored afterwards and
git status --porcelainverified empty.3. Mutation test — proof the gate is not the permanently-green no-op. The
mutation is the historical bug itself: the stale
"dev": trueonadm-zip@0.5.17was re-injected intopackage-lock.jsonand committed, so apristine checkout sees the rot exactly as CI would, with
package.jsonuntouched. Three commands were then run against that identical tree:
The mutant scores
exit=0on rot the shipped gate catches, so thenpm install --package-lock-onlyline is load-bearing and this test would failif it were removed.
npm cialso scoresexit=0, so the gate is not redundantwith the install step. The recovered hunk is the same
- "dev": true,linethat had to be cleaned up by hand in #564. The throwaway commit was dropped
(
git reset --hard HEAD~1) and the tree verified pristine.4. Workflow lints, parses, and preserves the rest of the file.
Asserted programmatically:
node-versionis the YAML string'24'(not afloat) and is the only input to
setup-node; the gate sits immediately afterInstall dependenciesand beforeCheck for secrets leaks, guarded bymatrix.os == 'ubuntu-latest'; andNODE_OPTIONS, the three-OS matrix and thesetup-python3.11 pin are unchanged.5.
npm cidoes not break the workspace layout.npm cideletesnode_modulesand rebuilds from the lock, so the threelink: trueworkspacesymlinks were re-checked and the pipeline re-run end to end on a clean tree:
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
To reproduce the gate firing, from a clean checkout on Node 24:
This PR is its own end-to-end test, and it has now run. The workflow runs on
pull_requestagainstmainand GitHub evaluates the workflow file from thePR's merge ref, so the change validates itself. Observed on this PR — all three
legs green:
ubuntu-latestnpm ci)windows-latestnpm ci)macos-latestnpm ci)From the
ubuntu-latestlog, confirming the pin resolved and the gate ran asdesigned:
That last line is the
bash -ethis step'sif ! …wrapper exists for, visiblein the real runner rather than inferred. (The runner resolved
24.18.0; localverification above used
24.18.1— same pin, one patch apart.)One flake, ruled out as unrelated. The first attempt failed on
macos-latestand was fail-fast-cancelled onwindows-latest, both inRun tests and check code coverage— never in a step this PR adds. The causewas
tests/integration/app_loader/app_loader_test.tshitting the 40s vitesttimeout (2677/2678 passed). This is pre-existing and independent of this
change: the identical
app_loader_test.tstimeout failed run30707362154onthe unrelated branch
feat/apihub-toolset-part1, which touches no CI config andstill ran
npm installunder an unpinned Node. A plain re-run of the failedjobs went green on all legs with no code change. It has been filed as separate
follow-up work rather than papered over 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.