Fix: install CI dependencies with npm ci so the committed lockfile is authoritative - #338
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: install CI dependencies with npm ci so the committed lockfile is authoritative#338AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
npm install treats the committed package-lock.json as a suggestion: it may re-resolve semver ranges and rewrite the lockfile in place, so CI can pass against a tree that no contributor's clean install reproduces. npm ci installs strictly from the lockfile, refuses to mutate it, and aborts with a named diagnostic when package-lock.json and any workspace manifest disagree. A package.json edit landed without its regenerated lockfile now fails at the install step instead of going green.
This was referenced Jul 30, 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
Link to an existing issue (if applicable):
No tracking issue exists for this change.
Or, if no issue exists, describe the change:
Problem: Both CI workflows install dependencies with
npm install, which treats the committedpackage-lock.jsonas a suggestion rather than a specification.npm installmay re-resolve semver ranges and it rewrites the lockfile in place — silently, since CI never reports the edit and throws the runner workspace away.Two consequences, both of which make the CI signal weaker than it looks:
package.jsonwithout committing the regenerated lockfile goes green.mainthen carries a lockfile that does not describe the tree, and the next contributor's clean install gets something different.^range can be picked up by CI while the lockfile still pins the old version, so a breakage is attributed to the PR under test rather than to the upgrade.This is not hypothetical for this repo. On a clean checkout of the base commit (
1210acc7) with nonode_modules, a plainnpm installmodifies the committed lockfile:No version changed in that particular run — the point is the write. Under
npm installthe committed lockfile is not authoritative in CI.Solution: Change the
Install dependenciesstep in both workflows fromnpm installtonpm ci. That is the entire diff — two files, two lines:npm ciinstalls strictly from the lockfile, refuses to mutate it, and aborts with a named diagnostic ifpackage-lock.jsonand any of the four workspace manifests (package.json,core/,dev/,integrations/) disagree.The precondition holds: exactly one lockfile is tracked (
git ls-files '*package-lock.json'→package-lock.json), it islockfileVersion: 3with 1167packagesentries, and it is already in sync —npm ciexits 0 on a clean checkout (see Testing Plan).Deliberately not included, to keep the diff to the one decision under review:
--no-audit/--no-fund/--prefer-offlineflags on the CI step. Each is a separate opinion and would muddy the before/after install-duration comparison below.continue-on-errorand no|| npm installfallback. The hard failure is the feature.actions/setup-node@v6with nowith:block, so they get the runner default, which is well past the npm 7 thatlockfileVersion: 3needs. Pinning Node is a separate change with its own tradeoffs.package-lock.jsonis byte-identical tomainin this PR (git diff main --statshows only the two workflow files). The stale"dev": trueonnode_modules/adm-zipvisible in the diff above is a real pre-existing latent bug —adm-zipis a production dependency ofcore(core/src/skills/loader.ts), sonpm ci --omit=devwould drop it — but it does not affect this change (CI never passes--omit=dev, and thedevflag is metadata that is not part of npm's sync check), and fixing it here would put a 562 KB lockfile diff in front of a two-line review. It is tracked separately.CONTRIBUTING.mdis untouched. It tells local contributors to runnpm install, which remains correct: developers need to be able to add a dependency and have the lockfile update. This change is CI-only.tests/integration/**fixtures are untouched. Several suites run their ownnpm installfrom inside test code against fixture projects that have no committed lockfiles (tests/\*\*/package-lock.jsonis gitignored), sonpm cidoes not apply there.Behaviour change contributors should know about. A PR that edits any
package.jsonwithout committing the regeneratedpackage-lock.jsonwill now fail at the install step instead of going green. The fix isnpm install && git add package-lock.json. The failure is self-describing:Collision check (run before writing anything).
gh pr list --state open --limit 100on the fork, thengh pr diff --name-onlyon every plausibly adjacent PR. Six open PRs touch a workflow file — #296 and #306 (cache: npmon thesetup-nodestep), #250 (deps:checkstep), #237 (a newintegration:slowjob), #235 (fail-fast), and #245 (root scripts only) — and none of them changesnpm installtonpm ci; grepping each diff fornpm ci/npm install/Install dependenciesreturns nothing but #237's new job, which adds anothernpm installrather than replacing one. So this change is uncontested and is branched frommainrather than stacked. The two cache PRs edit theUse Node.jsstep, a different step from the one edited here, and compose cleanly either way:npm cireads the same npm HTTP cachenpm installdoes, so whichever lands first, the other still applies unchanged. #237's new job would need the samenpm install→npm citreatment if both land; that is a one-line follow-up on whichever merges second, not a conflict.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.
New-code coverage: N/A — CI configuration change, no product code added. No test file and no Vitest project was added to manufacture a coverage number for a two-line YAML edit. The existing suite still gates the change:
npm run test:coverageruns unchanged invalidation.yamlimmediately after the install step, so any regression in the installed tree surfaces as an ordinary test failure.What replaces a coverage figure is a negative test and its counterfactual — the equivalent of proving a test can fail. Both were run locally on Linux, Node v22.22.2 / npm 9.2.0, from a clean checkout of this branch.
1. The sync check passes (the precondition).
npm's own summary count is not stable across runs (a second identicalnpm cireportedadded 1081 packagesfor a byte-identical tree), so the exit code is the assertion, not the count.2. The mutation — the guard actually fires. Adding a bogus
"left-pad": "^1.3.0"tocore/package.json'sdependenciesand re-running the same command:exit 1. So the pass in (1) is a real pass across all four manifests, not a check that never runs.
3. The counterfactual — the old command passes the same mutation. With
core/package.jsonstill drifted, the command this PR replaces:This is the whole argument in three lines: identical drifted input,
npm installgoes green and rewrites the lockfile,npm cifails loudly. The scratch edit tocore/package.jsonand the resulting lockfile write were both reverted (git checkout core/package.json package-lock.json); neither is in this PR.4. The installed tree does not change. Comparing the trees produced by
npm ciand bynpm installfrom an emptynode_modules—name@versionfor every package at every nesting depth, 1089 entries each —diffreports no differences, and a secondnpm cireproduces the first byte for byte. So this PR does not alter what CI tests today; it removes the mechanism by which that could silently drift.5. The tree works, and the CI gates pass on it. All run against the exact commit pushed, after a
npm ciinstall:npm run buildnpx vitest run --project unit:core core/test/skills/loader_test.tsadm-zip, a lockfile-resolved production dep)npm run lintnpm run format:checkAll matched files use Prettier code style!npm run docs:checknpx secretlint <the two workflow files>Per the repo's testing guidance the full suite was not run locally; CI runs it. (
npm run ts:checkfails on this branch, but it fails identically onmain— it is not a step in either workflow, and this diff contains no TypeScript.)6. The
preparescript is unaffected. The rootpreparescript (husky) runs undernpm ciexactly as undernpm install— visible in the install output above — so git-hook setup behaviour is unchanged.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
The PR's own CI run is the e2e test. To verify:
Confirm
validationis green onubuntu-latest,windows-latestandmacos-latest, andCross-Language Testsis green onmacos-latest.Expand the
Install dependenciesstep on each leg and confirm it runsnpm ciand logs no lockfile-rewrite warning.Compare install-step durations against the last
mainrun, which still usesnpm install, via the Actions API rather than the UI:To reproduce steps 1–3 of the Testing Plan locally, run the commands as given from a clean checkout; step 2 requires temporarily adding a package to a workspace manifest that is not in the lockfile, and step 3 requires reverting
package-lock.jsonafterwards.Results of this PR's own CI run (validation run
30582056937, Cross-Language run30582056925):validationpasses on all three matrix legs —ubuntu-latest,windows-latest,macos-latest.Cross-Language Testspasses onmacos-latest.The
Install dependenciesstep succeeded on all four legs. The ubuntu log shows exactly what it should:No lockfile-rewrite warning, no
EUSAGE, and theprepare/huskyhook installs normally. Runner toolchain: node v22.23.1, npm 10.9.8.One Windows flake, unrelated to this change, and proven so. The first Windows attempt failed in
tests/integration/tools/run_skill_script_tool_test.ts—successfully executes a real PowerShell skill scriptandcaptures stderr from a failing PowerShell skill script, bothTest timed out in 5000ms. Re-running that job on the identical commit, with no changes, passed. The margin is the cause: on another current PR's Windows leg (which still usesnpm install) the same PowerShell test passes in2719msagainst the same 5000 ms budget. This is a latency-sensitive test on a slow shell, not an install-method effect — the installed tree is identical either way (§4) — and it is already being addressed by separate open PRs about PowerShell spawn cost and Windows shell test timeouts.Install-step duration, before vs after, per OS. Pulled from the Actions API (
/actions/runs/<id>/jobs,Install dependenciesstarted_at/completed_at), not the UI.npm install)npm ci)validationubuntu-latestvalidationmacos-latestvalidationwindows-latestCross-Language Testsmacos-latest⁽¹⁾ From run
29888027719(main@85d0321), because the Windows leg of themainrun at this PR's base commit (30405795959,1210acc7) was cancelled by matrix fail-fast and recorded no step timings. The ubuntu/macos "before" figures are from that base-commit run.⁽²⁾ Two attempts of the same commit: 84 s on attempt 1, 36 s on attempt 2.
Read this as an observation, not a benchmark. One sample per cell, and the Windows cell alone varies by a factor of 2.3 between two runs of an identical commit — a range that brackets the baseline, so no conclusion about Windows is available from this data. Ubuntu and macOS are flat within noise. Locally the two verbs were also indistinguishable (
npm ci15.5 s vsnpm install16.0 s from an emptynode_moduleswith a warm HTTP cache). This change is not a speed optimization and none is claimed — the justification is determinism.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 fourth box: this change adds no product code to unit-test, so what is offered instead is the negative test in §2 plus its counterfactual in §3 — the drifted-manifest input that
npm cirejects andnpm installaccepts. That pair is what proves the change is effective.