test: apply the supply-chain age gate to npm and pnpm in Verdaccio tests - #4347
Draft
erickzhao wants to merge 10 commits into
Draft
test: apply the supply-chain age gate to npm and pnpm in Verdaccio tests#4347erickzhao wants to merge 10 commits into
erickzhao wants to merge 10 commits into
Conversation
The Verdaccio tests install the freshly published monorepo into throwaway app directories, so they can never pick up the root `.yarnrc.yml`. Until now only Yarn's gate was mirrored in the harness; npm and pnpm installed without one. Both have since grown the same policy, so set it for them too and keep the shared values in one place: - npm calls it `min-release-age` (in days) and has supported it since 11.19, so only pass it when the npm on `PATH` is new enough and say once when it isn't. CI pins npm 11.19.0 because npm 12 requires a newer Node than `.nvmrc`. - pnpm calls it `minimumReleaseAge` (in minutes) but reads it from its config files only, so generate a global config and point pnpm at it with `XDG_CONFIG_HOME`. That config also has to set `registry`: pnpm honors `--registry` and its config files but not `npm_config_registry`, so the pnpm half of these tests was resolving `@electron-forge/*` from the public registry and validating the last published release instead of the local build. Two other fixes fall out of this: - `pnpm store prune` is gone in favour of a per-run `cacheDir` under the storage directory. Staleness lives in the metadata cache, not in the content-addressed store, and a cold store made pnpm hang after installing until the test runner timed out. - `COREPACK_ROOT` no longer leaks into the spawned tests. `yarn test:verdaccio` runs Yarn through Corepack, and pnpm refuses to switch to the version `create-electron-app` pins when it thinks Corepack invoked it. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
pnpm shuts its tarball worker pool down once per install, but any worker call that happens after that lazily creates a new pool that nothing ever shuts down, and the idle worker thread keeps the event loop alive. An install whose last download finishes just after pnpm prints `Done in Xs` therefore writes the lockfile, links everything, reports success and then never exits (pnpm/pnpm#13617). Installs that fetch nothing are unaffected, which is why this only shows up in these tests: they always install into a brand new project. Every test here spawns its package manager and waits for it to exit, so the hang costs the whole test rather than just the process, which is what was timing out the `pnpm` template tests on all three platforms. Put a stand-in for pnpm at the front of `PATH` for the duration of these tests that kills it once it has reported that it is done and has had a grace period to exit on its own. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Since pnpm 11, `pnpm run` silently runs an install first whenever it decides that `node_modules` is out of sync with the lockfile. These tests run `<package manager> run start` to check that the app `create-electron-app` just installed can start, so that install replaces the very thing they are checking: on Windows it rewrote the dependency tree into one where the generated `forge.config.ts` could no longer resolve the Forge plugin it imports, and `electron-forge start` failed. Set `verifyDepsBeforeRun` to `warn` so the check still runs and still reports whatever it believes is out of sync, without acting on it. While here, only watch for the exit hang on the commands that install packages. `pnpm run start` keeps running long after pnpm reports that it is done, and the last thing the exit shim should do is kill it. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
pnpm decides whether to link dependencies through a store-wide virtual store or one inside the project based on whether it believes it is running in CI, and these tests cannot keep that consistent: they install with the environment they inherit and then run the app's `start` script with a minimal one, so pnpm read the same project two different ways and reported that `node_modules` no longer matched the lockfile. Pin the setting to the value CI would pick anyway. Also report the dependency tree the package manager installed when `start` fails, since a `start` that cannot resolve the app's own configuration says nothing about the tree it was trying to resolve it from. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The shim this removes ran pnpm and killed it 15 seconds after it printed `Done in`, on the theory that anything past that point was the leaked worker pool of pnpm/pnpm#13617 keeping a finished process alive. On Windows it was cutting installs short instead: both TypeScript templates came out of `create-electron-app` with exactly the base template's dependencies and none of their own, because the second install reports that it is done and is then killed while the very work the pnpm bug defers — copying packages into `node_modules` — is still going. Windows CI keeps its pnpm store on `C:` and its projects on `D:`, so nothing can be hardlinked and that tail takes far longer than it does anywhere else. An install that quietly loses half of a project is a worse failure than one that hangs, and the hang the shim was written for was in the install `pnpm run` used to perform behind the tests' back, which `verifyDepsBeforeRun: warn` already stopped. Also report what `create-electron-app` printed when `start` fails, since it runs its steps with listr2's `exitOnError: false` and so exits 0 with a broken project when an install fails. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Restores the watchdog removed in the commit before this one — pnpm really does finish an install and then hang, and without it the vite-typescript project times out at 240s — but stops it from leaving a project behind that cannot be repaired. Killing pnpm once it reports success is only safe if it has really stopped working, and it hasn't: the hang comes from a package that arrived after pnpm stopped expecting one, and that package is still being written. Worse, pnpm will not put it back, because it decides whether a project is up to date from the state files it keeps in `node_modules` rather than from the packages themselves — installing again into a tree it has already recorded is `Already up to date` even with packages deleted out of it, `--force` included. That is what Windows was failing on: both TypeScript projects came out of `create-electron-app` with the base template's dependencies and none of their own. So the watchdog now discards those state files after killing pnpm and installs again, which makes pnpm compare the tree against the store and write whatever is missing. A repair only has to link packages that are already in the store, so it is very unlikely to hang in turn; if every attempt does, the shim now says so and fails instead of reporting a success it cannot vouch for. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Windows keeps producing projects whose `node_modules` has the base template's dependencies and none of the template's own, and nothing says why: `create-electron-app` installs through listr2 with `exitOnError: false` and never prints the errors it collects, so a failed install leaves the task without a tick and without a word about it, and the package manager's own output is thrown away with it. So the pnpm shim now records every pnpm it runs — the command, the directory, how it ended and everything it printed — and a test whose app fails to start reports the runs for its own project, along with the `package.json` they were working from, which is what says whether the dependencies were recorded and not installed or never recorded at all. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
… passes on On Windows the real pnpm is a `.cmd` file, so the shim ran it through `cmd.exe`, which Node hands the command line to unquoted. `cmd.exe` reads `^` as its own escape character, so `pnpm add typescript@^6.0.0` reached pnpm as `typescript@6.0.0` — a version that does not exist — and the install failed with `ERR_PNPM_NO_MATCHING_VERSION`. Every other dependency the templates ask for happens to have a release at exactly the version its range starts from, which is why only the two TypeScript templates failed, and why the projects they left behind had `"@electron/fuses": "2.0.0"` where every other platform gets `"^2.1.3"`. `create-electron-app` runs its steps with listr2's `exitOnError: false` and never prints the errors it collects, so all of this was silent: the install failed, the project was left without the template's own dependencies, and the test only found out when the app could not resolve its Forge configuration. Hand the spawning to `cross-spawn`, which quotes and escapes arguments the way `cmd.exe` needs and is what Forge itself runs package managers with. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
The manifest entry was left out of the commit that started importing it, so installs with a frozen lockfile failed. `^7.0.3` is the range the rest of the project asks for, which `yarn constraints` requires it to match. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
`create-electron-app` pins every pnpm app it creates to the pnpm version this repository runs in CI, and until pnpm 11.18 adding a dependency to a project that already had some could drop a package that another package it kept still depends on. The last of the four installs `create-electron-app` runs left `rimraf` in `node_modules` without the `glob` it requires, so `forge.config.ts` could no longer load and the Verdaccio template tests could not start the app they had just created. It only showed up on Windows, for two reasons: everywhere else the platform-specific makers' dependencies pull `glob` in through a second path that keeps it in the tree, and Corepack pins `pnpm@latest` for the app it creates, which is much newer than this pin. Corepack fails on the Windows runners, so there the pin is what actually installs. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
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
1 participant
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.
The root
.yarnrc.ymlsetsnpmMinimalAgeGatewith an exemption list for our own packages. The Verdaccio tests install the freshly published monorepo into app directories underos.tmpdir(), so they never pick that file up, and the harness only ever mirrored the Yarn half of the policy — npm and pnpm installed with no gate at all. Both have since grown the same setting, so this mirrors it for them too, with the age and the exemption list defined once and spelled three ways.min-release-ageand counts it in days. It landed in 11.19, and older versions warn "unknown config" on every single invocation, so the harness only passes it when the npm onPATHsupports it and warns once when it doesn't. CI installs npm 11.19.0 for the slow-tests job — npm 12 requires Node^22.22.2 || ^24.15.0 || >=26and.nvmrcpins 22.13, so 12 can't be installed there at all.minimumReleaseAgeand counts it in minutes, but only reads it frompnpm-workspace.yamlor the global config file. Writing apnpm-workspace.yamlinto each generated app would make Forge's ownresolvePackageManagermisdetect the npm and Yarn cases, so the harness generates a global config under the Verdaccio storage directory and points pnpm at it withXDG_CONFIG_HOME.The pnpm leg wasn't using the local registry
That generated config also has to set
registry. pnpm honors--registryand its config files, but it ignoresnpm_config_registryfrom the environment, which is how every other package manager in the harness is pointed at Verdaccio. So the pnpm third of these tests has been resolving@electron-forge/*from the public registry and validating the last published alpha rather than the local build.Verified by baking a marker into
packages/api/cli/dist, publishing, and diffing what got installed: before the fix npm and Yarn got the marker and pnpm didn't, and pnpm's lockfile integrity matched the npmjs tarball. After it, pnpm's integrity matches the locally published one.Two fixes that fall out of it
pnpm store pruneis replaced by a per-runcacheDirunder the storage directory, whichstartVerdaccioalready deletes. The prune was there so a republished version couldn't resolve through stale metadata, but staleness lives in the metadata cache, not in the content-addressed store — and a fully cold store made pnpm hang after installing (its worker pool never shut down) until the test runner timed out. Confirmed by running twice with different markers: the warm store serves the fresh build both times.COREPACK_ROOTleakage.yarn test:verdaccioruns Yarn through Corepack, which exportsCOREPACK_ROOTto everything below it, including the package manager installing each generated app. pnpm refuses to switch to the version it's asked for when it believes Corepack invoked it, andcreate-electron-apprunscorepack use pnpm@latest, so every pnpm app failed its first install with a version mismatch. The harness now drops the variable.Testing
yarn test:verdaccio— the real entry point, through Corepack Yarn — passes 64/64 across all 10 files.Noticed but not changed
exitOnError: falseon the "Installing common dependencies" task atpackages/external/create-electron-app/src/init.ts:248means a failed dependency install is swallowed by listr2: the CLI exits 0 and leaves the user with a scaffold that has no dependencies. That's what made theCOREPACK_ROOTbug above so hard to see, and real users hitting an install failure get the same silent success. Worth fixing separately, since it changes user-facing CLI behavior.🤖 Generated with Claude Code
Co-Authored-By: Claude svc-devxp-claude@slack-corp.com