Skip to content

Fix: fail CI when package-lock.json is not a fixed point (and correct the stale adm-zip dev flag) - #345

Open
AmaadMartin wants to merge 2 commits into
feat/ci-npm-ci-deterministic-installfrom
fix/ci-npm-ci-lockfile-drift
Open

Fix: fail CI when package-lock.json is not a fixed point (and correct the stale adm-zip dev flag)#345
AmaadMartin wants to merge 2 commits into
feat/ci-npm-ci-deterministic-installfrom
fix/ci-npm-ci-lockfile-drift

Conversation

@AmaadMartin

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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Stacked PR. Base is feat/ci-npm-ci-deterministic-install (the npm installnpm ci switch), not main. See "Collision check" below.

Problem: The committed package-lock.json is not a fixed point of the resolver, and CI cannot tell.

  • The lock is wrong. node_modules/adm-zip carries "dev": true. That flag is semantically incorrect: adm-zip is declared under dependencies in core/package.json (line 63) and is imported by shipped runtime source at core/src/skills/loader.ts:7 (import AdmZip from 'adm-zip';). It is a devDependencies entry only of the dev workspace (dev/package.json:48). Any resolve that honours the flag — npm ci --omit=dev, a production install driven by this lock — would omit a package that runtime code imports.
  • CI cannot detect it. npm install recomputes the ideal tree and silently repairs the flag in the runner's working copy; nothing ever inspects the working tree afterwards, so the repair is thrown away with the runner and CI stays green. Switching the install to npm ci (this PR's base) closes the determinism hole but does not catch this defect class: npm ci validates that the lock satisfies the declared ranges, not that the lock is a fixed point. Measured: npm ci exits 0 on the stale lock.

Solution: two changes, kept in one PR because the second cannot land green without the first.

  1. Regenerate the lock to a fixed point — produced with npm install --package-lock-only, never hand-edited. The diff is exactly one deleted line, the "dev": true, on node_modules/adm-zip. node_modules/@types/adm-zip correctly keeps its "dev": true (it really is a devDependencies entry of core).

  2. Add a drift guard to .github/workflows/validation.yaml — re-resolve the lock and fail on any diff:

    - name: Check package-lock.json is up to date
      if: matrix.os == 'ubuntu-latest'
      run: |
        npm install --package-lock-only
        git diff --exit-code package-lock.json

    Design notes:

    • Placed immediately after the install and before every other step, so a drifted lock fails in seconds rather than after the full build-and-test cycle.
    • Gated to ubuntu-latest: lock content is OS-independent, so one leg is sufficient coverage.
    • Hard failure by construction — no continue-on-error, no || true, no warn-only mode. A guard that only warns reproduces the very defect being fixed.
    • The remedy a contributor needs is the command the guard itself runs: npm install --package-lock-only, then commit the result.
    • No --ignore-scripts / --no-audit / cache: 'npm' / node-version pin was added; toolchain pinning and caching are out of scope here.

Deliberately unchanged: CONTRIBUTING.md keeps recommending npm install for local development — contributors legitimately need it to update the lock; npm ci is a CI concern. No package.json is modified: this fixes the lock, it does not move any dependency between dependencies and devDependencies. .github/workflows/cross-language-integration.yml is not touched by this PR (the base PR already switches its install).

Impact on the installed tree: none. The dev flag only influences --omit=dev installs, and CI installs with dev dependencies included. npm ci installs 1089 packages both before and after this change.

Collision check (open PRs on the fork, gh pr list --state open --limit 100, then gh pr diff --name-only on every adjacent hit):

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.

Coverage note: this change adds zero lines of executable product code — it is one workflow step and one resolver-generated lockfile line — so the new-line-coverage gate is not applicable and no test file was added. No existing test was modified, skipped, weakened or deleted; vitest.config.ts is untouched. The workflow is the artifact under test, so it is verified by mutation instead: each check below was run against the unfixed state first and shown to FAIL.

Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, see coverage note. Targeted regression run of the adm-zip consumer instead: npm test -- core/test/skills/loader_test.ts30 passed.
[x] All unit tests pass locally.

Mutation proof 1 — the guard fails on a drifted lock (red). With the stale "dev": true committed:

$ npm install --package-lock-only        # exit 0
$ git diff --exit-code package-lock.json
@@ -5207,7 +5207,6 @@
        "integrity": "sha512-+Ut8d9LLqwEvHHJl1+PIHqoyDxFgVN847JTVM3Izi3xHDWPE4UtzzXysMZQs64DMcrJfBeS/uoEP4AD3HQHnQQ==",
 -      "dev": true,
        "license": "MIT",
exit=1

Re-verified as a true mutation against this branch: re-injecting "dev": true onto node_modules/adm-zip and committing it makes the guard fail with the same one-line diff and exit=1; the scratch commit was then discarded. (Injecting it into the working tree only is not a valid mutation — the guard's own npm install --package-lock-only repairs it and git diff is then correctly empty.)

Mutation proof 2 — the guard passes on the fixed lock (green). Same two commands on this branch print nothing and exit=0.

Mutation proof 3 — npm ci is clean and non-mutating.

$ rm -rf node_modules && npm ci        # exit 0, "added 1089 packages in 16s", husky prepare ran cleanly
$ git status --porcelain package-lock.json
                                       # empty

Mutation proof 4 — npm ci still fails loudly on a real range disagreement (guard-the-guard). Temporarily setting core/package.json's adm-zip range to ^0.6.0:

npm ERR! code EUSAGE
npm ERR! `npm ci` can only install packages when your package.json and package-lock.json ... are in sync.
npm ERR! Missing: adm-zip@0.6.0 from lock file
exit=1

Edit reverted; not committed. This proves the install step retains its validating power and was not silently degraded.

Full local gauntlet on the exact pushed commit (Linux, Node v22.22.2, npm 9.2.0):

Command Result
npm ci exit 0 (1089 packages)
npm install --package-lock-only && git diff --exit-code package-lock.json exit 0
npm run build exit 0
npm run lint exit 0
npm run format:check exit 0
npm run docs:check exit 0
npx secretlint on both changed files exit 0
npm test -- core/test/skills/loader_test.ts 30/30 passed

CI status: absent, validated locally instead. This is a stacked PR whose base is feat/ci-npm-ci-deterministic-install, and validation.yaml triggers on pull_request: branches: [main] — so the workflow does not run for this base and no test job will ever appear on this PR. The table above is the substitute evidence, run on the exact commit pushed. The workflow change itself takes effect once the stack lands on main.

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 whole chain from a clean checkout of this branch:

  1. npm ci — must exit 0 and leave git status --porcelain package-lock.json empty.
  2. npm install --package-lock-only && git diff --exit-code package-lock.json — must exit 0 (this is verbatim what the new CI step runs).
  3. To watch the guard bite, delete any line from a lock node — e.g. re-add "dev": true, to node_modules/adm-zipcommit it, then repeat step 2: it exits 1 and prints the offending node.

Note for local reproduction: npm run docs:check exits 3 with The provided git remote "origin" was not valid if the checkout has no origin remote. actions/checkout always configures one, so this is a local-scratch-directory artifact, not a regression.

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. — see the coverage note and the four mutation proofs above; no product code was added, so the verification is the red/green mutation protocol rather than a new test file.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits July 30, 2026 16:10
`node_modules/adm-zip` carried a stale `"dev": true` flag. adm-zip is a
runtime `dependencies` entry of `core/package.json` and is imported by
shipped source (`core/src/skills/loader.ts`); it is only a devDependency
of the `dev` workspace. The lock had recorded the dev-only view, so an
`--omit=dev` resolve driven by this lock would drop a package that
runtime code imports.

Produced with `npm install --package-lock-only`, not by hand-editing.
The installed tree is unchanged: the flag only affects `--omit=dev`
installs, and CI installs with dev dependencies included.
npm ci makes the install deterministic, but it only checks that the lock
can satisfy the declared ranges -- it exits 0 on a lock that the resolver
would still rewrite. That is exactly the drift class that went unnoticed
here, so the switch alone does not close the hole.

Re-resolve the lock and fail on any diff. Runs immediately after the
install so a drifted lock fails in seconds rather than after the full
build-and-test cycle. Gated to ubuntu-latest: lock content is
OS-independent, while the platform-constrained optional nodes and the
absence of .gitattributes make the other two legs pure downside.

The remedy for a failure is the command the guard itself runs:
`npm install --package-lock-only`, then commit the result.
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