Build: Detect stale node_modules at build/dev time#77995
Merged
Conversation
manzoorwanijk
requested review from
Mamaduka,
aduth,
ciampo,
jsnajdr,
mirka,
sirreal,
t-hamano and
tyxla
manzoorwanijk
added
the
[Type] Build Tooling
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in d5ae5b8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25429739923
|
|
Size Change: 0 B Total Size: 7.95 MB ℹ️ View Unchanged
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic “stale node_modules” guard for Gutenberg’s local build/dev flows by comparing the declared lockfile (package-lock.json) against npm’s installed-tree lockfile (node_modules/.package-lock.json). It replaces the previous TypeScript-only drift check so developers get a fast, actionable failure (“run npm install”) when dependencies are out of sync.
Changes:
- Add a new validation workspace (
tools/validation) with acheck-installed-depsscript that checks lockfile vs installed-tree integrity. - Run the new dependency sync check as Step 0 in
bin/build.mjsandbin/dev.mjs, and inbuild:profile-types. - Expand the ESLint “allow console” override to include
tools/**/*.js|mjs.
Reviewed changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/validation/package.json | Adds a new private workspace package to host validation scripts. |
| tools/validation/check-installed-deps.mjs | Implements the lockfile vs installed-tree drift detection logic. |
| tools/eslint/config.mjs | Allows console usage in tools/** scripts (consistent with CLI/tooling usage). |
| package.json | Switches build:profile-types to run the new dependency sync check. |
| package-lock.json | Registers @wordpress/validation-tools as a workspace link. |
| bin/build.mjs | Runs dependency sync validation as Step 0 before the rest of the build. |
| bin/dev.mjs | Runs dependency sync validation as Step 0 before starting the dev build/watch. |
| bin/packages/validate-typescript-version.js | Removes the legacy TypeScript-only drift guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
manzoorwanijk
added a commit
that referenced
this pull request
May 6, 2026
Three small fixes from #77995's review: - Track total mismatch count separately from the printed list, so the reported count is accurate when more than MAX_REPORTED packages drift. Verbose output now appends "... and N more." when truncated. - Rename the verbose label `version mismatch` -> `integrity mismatch`, since the comparison is on the integrity hash (not the semver string). - Move and reword the optional-deps comment to sit alongside the branch it describes, and clarify that real drift on optional deps is still caught by the integrity check below.
Member
|
It'd be nice if we could have some kind of GNU Make-like skip procedure if the timestamps of |
aduth
reviewed
May 12, 2026
Comment on lines
+93
to
+94
| '--workspace', | ||
| '@wordpress/validation-tools', |
Member
There was a problem hiding this comment.
Is it necessary to specify --workspace here?
Member
Author
There was a problem hiding this comment.
Not necessarily, but it abstracts out the implementation details from the build script, reverting it loses the benefit of isolation, which you rightly pointed out.
The cold start via npm run is actuall the npm problem, which I am not sure we can do anything about.
Comment on lines
+32
to
+40
| if ( ! fs.existsSync( HIDDEN_LOCKFILE ) ) { | ||
| fail( | ||
| 'node_modules is missing or incomplete.', | ||
| `\t${ path.relative( ROOT, HIDDEN_LOCKFILE ) } not found.` | ||
| ); | ||
| } | ||
|
|
||
| const lock = JSON.parse( fs.readFileSync( LOCKFILE, 'utf8' ) ); | ||
| const hidden = JSON.parse( fs.readFileSync( HIDDEN_LOCKFILE, 'utf8' ) ); |
Member
There was a problem hiding this comment.
Some non-blocking micro-optimization ideas:
- We can skip the stats call on
HIDDEN_LOCKFILEby putting the read + parse into atryand failing with the same message in acatch.- FWIW,
existsis deprecated anyways
- FWIW,
- We could parallelize the file reads with a
const [ lock, hidden ] = Promise.all( [ LOCKFILE, HIDDEN_LOCKFILE.map( ( ... ) => ... ) - If we wanted to go extra hardcore, we could consider some sort of streaming approach, e.g.
stream-json
Member
Author
There was a problem hiding this comment.
Good suggestion. Thanks
Implemented in 7a99f89
Member
|
It might be worth temporarily opening up the |
aduth
approved these changes
May 12, 2026
Member
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
Feedback above, but generally this looks good and tests well for me locally 👍
manzoorwanijk
added a commit
that referenced
this pull request
May 13, 2026
Per @aduth's review on #77995: cache the mtimes of package-lock.json and node_modules/.package-lock.json after a successful run, and skip the full integrity scan on subsequent invocations when neither has changed. Cache lives in node_modules so a fresh install wipes it. `--verbose` always forces a fresh check so debug output reflects the current state. Cache writes are best-effort — a write failure just means we re-check next time. Turns the steady-state (no-changes) case into a no-op so the dep check stops being a per-build tax.
manzoorwanijk
added a commit
that referenced
this pull request
May 13, 2026
Per @aduth's review on #77995: - Drop the fs.existsSync probe on the hidden lockfile; rely on the stat that we needed anyway and detect ENOENT in the catch. - Run the two stat calls in parallel via Promise.all (cheap, but worth doing on the hot path). - Run the two file reads in parallel via Promise.all on the cache-miss path — saves one disk roundtrip on cold runs.
manzoorwanijk
added a commit
that referenced
this pull request
May 13, 2026
Per @aduth's review on #77995: temporarily drop the PR-only matrix exclusions in .github/workflows/static-checks.yml so the new installed-deps check runs on the full Node 20/22/24 x Linux/macOS/ Windows grid. The hidden-lockfile format is an npm implementation detail, so verifying behavior across npm versions and on macOS CI catches regression risk before merge. REVERT THIS COMMIT before merging.
Member
Author
Addressed in 33d2b6f. It now writes the last check to a cache file in |
Replace bin/packages/validate-typescript-version.js with a generic install-drift check that catches stale node_modules from any dependency, not just typescript. The new script (tools/validation/check-installed-deps.mjs) compares package-lock.json against npm's hidden lockfile (node_modules/.package-lock.json) and verifies installed integrity per registry-fetched entry, ignoring workspace links and platform-skipped optional deps. Set up tools/validation/ as a workspace (@wordpress/validation-tools, picked up by the existing tools/* glob). Build, dev, and build:profile-types invoke the check via `npm run check-installed-deps --workspace @wordpress/validation-tools`. Aligns with the validation-tools workspace direction from #77522 and Also extends the CLI/bin/env eslint override (no-console: off) to cover tools/validation/**, mirroring the same change in #77522.
Three small fixes from #77995's review: - Track total mismatch count separately from the printed list, so the reported count is accurate when more than MAX_REPORTED packages drift. Verbose output now appends "... and N more." when truncated. - Rename the verbose label `version mismatch` -> `integrity mismatch`, since the comparison is on the integrity hash (not the semver string). - Move and reword the optional-deps comment to sit alongside the branch it describes, and clarify that real drift on optional deps is still caught by the integrity check below.
Per @aduth's review on #77995: cache the mtimes of package-lock.json and node_modules/.package-lock.json after a successful run, and skip the full integrity scan on subsequent invocations when neither has changed. Cache lives in node_modules so a fresh install wipes it. `--verbose` always forces a fresh check so debug output reflects the current state. Cache writes are best-effort — a write failure just means we re-check next time. Turns the steady-state (no-changes) case into a no-op so the dep check stops being a per-build tax.
Per @aduth's review on #77995: - Drop the fs.existsSync probe on the hidden lockfile; rely on the stat that we needed anyway and detect ENOENT in the catch. - Run the two stat calls in parallel via Promise.all (cheap, but worth doing on the hot path). - Run the two file reads in parallel via Promise.all on the cache-miss path — saves one disk roundtrip on cold runs.
Per @aduth's review on #77995: temporarily drop the PR-only matrix exclusions in .github/workflows/static-checks.yml so the new installed-deps check runs on the full Node 20/22/24 x Linux/macOS/ Windows grid. The hidden-lockfile format is an npm implementation detail, so verifying behavior across npm versions and on macOS CI catches regression risk before merge. REVERT THIS COMMIT before merging.
manzoorwanijk
force-pushed
the
build/check-installed-deps
branch
from
ebdcf32 to
451f2af
Compare
Member
Author
Good idea. I have temporarily done it in 451f2af. Let us see |
Member
Author
See to work fine - https://github.com/WordPress/gutenberg/actions/runs/25794750008/job/75769043609?pr=77995 |
…dation" This reverts commit 451f2af.
Member
Author
|
@aduth do you mind having another quick look at this? |
aduth
approved these changes
May 13, 2026
Member
There was a problem hiding this comment.
LGTM 👍 Worth flagging that this might cause a bit of friction for other maintainers, since npm run build will fail a lot more often when any package is out of sync. That seems like it's the intended purpose of the changes (i.e. build could produce wrong output or otherwise still fail), but worth higlighting.
Per @aduth's review on #77995: `--verbose` only adjusts output detail on failure, and a cache hit means there is nothing to be verbose about. Gating the cache check on `! verbose` served no purpose; cache now applies regardless of the flag. Also tightens the cache block — `cached` lives inside the try, so the no-cache / unreadable / mtime-mismatch fall-through is the single empty catch path.
Per @aduth's review on #77995: the cache-hit branch was duplicating the trailing `✔ All good.` log via an early `process.exit(0)`. Replace with a `needsCheck` flag — on cache hit, skip the full check and fall through to a single end-of-script log; on cache miss, run the check and write the cache before reaching the same log.
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.
What?
Inspired by #77950.
Replace the TypeScript-only install-drift guard (
bin/packages/validate-typescript-version.js, called frombin/build.mjs/bin/dev.mjs) with a generic lockfile-sync check that catches drift from any dependency, not justtypescript.The new script lives at
tools/validation/check-installed-deps.mjsand is invoked frombin/build.mjs,bin/dev.mjs, and thebuild:profile-typesscript.Why?
In the review of #77950 (this comment, finding 5), @ciampo flagged that removing
validate-typescript-version.jsloses a local-dev guardrail. Syncpack only checks declared values across workspaces — it does not detectnode_modulesbeing stale relative topackage.json/package-lock.json. A developer who pulls a branch that bumps TypeScript without re-runningnpm installwould now hit cryptictsgoerrors instead of the previous clear "Detected vs Required" message.The follow-up discussion settled on the right shape:
This PR is that follow-up — a wholistic check that flags any out-of-sync
node_modules, not just TypeScript.How?
The script compares
package-lock.json(declared) againstnode_modules/.package-lock.json(npm's hidden lockfile, rewritten on every install to record the actual installed tree). For every registry-fetched entry, it checks that both files agree onintegrity; mismatches and missing entries exit non-zero with a hint to runnpm install. Workspace links and platform-skipped optional deps are excluded to avoid false positives.It runs as the first step of
npm run buildandnpm run dev, so a stale install fails fast with a clear message instead of producing confusing downstream errors.Testing Instructions
All cases run locally; no wp-env required.
1. Happy path (in-sync install) — should pass
Expected: build prints
🔍 Checking dependencies...as Step 0 followed by✔ All good., then runs through the remaining steps and finishes with🎉 Build completed successfully!.2. Stale install — missing
node_modules/.package-lock.jsonmv node_modules/.package-lock.json node_modules/.package-lock.json.bak npm run build # Restore: mv node_modules/.package-lock.json.bak node_modules/.package-lock.jsonExpected: build aborts at Step 0 with:
3. Stale install — a package was removed since last install
Simulate without actually breaking your
node_modules:Expected: build aborts at Step 0 with:
4. Stale install — a package's version changed (integrity mismatch)
Expected: build aborts at Step 0 with:
5. Verbose mode — show technical details
The script supports a
--verboseflag for debugging that shows which specific packages are out of sync (or which path was missing). With one of the failure scenarios from tests 2-4 in place:Expected (e.g. for a single missing package):
6. Dev mode — check also runs as Step 0
npm run dev # (Ctrl-C once you see "🔍 Checking dependencies..."; no need to wait for the full watch.)Expected: prints
🔍 Checking dependencies...immediately after the "Starting development build..." banner.7. Lint
Expected: passes.
Screenshots or screencast
N/A — CLI-only change.
Use of AI Tools
This PR was authored with assistance from Claude Code (Anthropic). The script logic, workspace scaffolding, and PR description were drafted with AI assistance; all changes were reviewed, tested locally, and verified against a clean install plus the failure simulations described in the testing instructions above.