Skip to content

validate-peer-deps guard runs after npm ci, so it never fires on the failure it exists to diagnose #85

Description

@bautrey

Problem

scripts/validate-peer-deps.js (added in #84) catches intra-workspace dependency ranges that no longer resolve locally. In validate.yml it runs as a step after Install dependencies:

- name: Install dependencies
  run: npm ci
- name: Validate intra-workspace dependency ranges
  run: node scripts/validate-peer-deps.js

But an unresolvable range is exactly what makes npm ci fail. So on the drift this guard exists to catch, npm ci 404s first, the guard step never executes, and the contributor gets the bare registry error instead of packages/react (peerDependencies): @fortium/ensemble-core "^4.0.0" that workspace is at 5.3.0 — widen to "^5.0.0".

Apply the test that makes this obvious: what would this check look like if the thing it guards were broken? Identical — the job is red either way, and the guard contributes nothing to why. In CI it is currently decoration. Its real value today is local, when someone runs npm run validate before pushing.

Why it can't simply be reordered

The script does require('semver'), which resolves from node_modules. Moving the step above npm ci makes it crash on a missing module instead of running.

Suggested fix

Make the guard dependency-free so it can run as the first step in the job, before any install:

  • Implement the narrow range check the repo actually needs (caret, >=, exact, *) rather than importing semver.
  • Fail loudly on any range form the parser does not recognise. That is the whole safety property — an unknown form must error, never quietly pass. Same discipline as the workspaces pattern handling already in the script, which exits non-zero on a glob shape it cannot expand rather than silently checking fewer workspaces.
  • Then reorder: guard first, npm ci second. A contributor who breaks a range gets the named file and the suggested range in about a second, instead of a registry 404 they have to decode.

Deliberately not done in #84: hand-rolling range satisfaction is new capability, and that PR's job was to unbreak npm ci, not to grow.

Context

Found by the silent-failure-hunter during #84's review loop. Related: #83, same shape — a check whose condition is a proxy for the thing actually meant.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Priority P2bugSomething isn't workingmedium

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions