chore: drop the packageManager pin and relocate pnpm overrides - #96
Merged
Conversation
`packageManager` made pnpm self-manage its own version: every invocation downloaded pnpm 10.30.3 from the registry and re-exec'd into it, so the pnpm that ran was never the one the flake provided. Offline that fails outright, and there is no opt-out. Removing the pin moves the project onto the flake's pnpm 11, which no longer reads the `pnpm` field in package.json. The DoS overrides from the js-yaml and markdown-it advisories move to pnpm-workspace.yaml, where both pnpm 10 and 11 read them.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Summary
Remove the
packageManagerpin frompackage.jsonso pnpm comes from the Nix flake instead of the registry. Moving off pnpm 10 forces the DoS advisoryoverridesintopnpm-workspace.yaml, because pnpm 11 no longer reads thepnpmfield inpackage.json.Why the pin has to go
pnpm 10+ self-manages its version from this field by default: every invocation downloads that exact pnpm from the registry and re-execs into it, so the pnpm that runs is never the one the environment provides. The same flake binary, with and without the field:
Offline it fails outright (
[ERROR] GET https://registry.npmjs.org/pnpm: fetch failed). There is no opt-out:manage-package-manager-versions=falsein a project.npmrcis read back correctly bypnpm config get, yet pnpm switches anyway. Matching the pin to a developer's pnpm is no fix either, since pnpm skips the download only on an exact version match, so the next upgrade re-triggers it. Absence of the field is the only stable state.Design decisions
overridesmove rather than being duplicated. pnpm 11 ignorespnpm.overridesinpackage.json([WARN] The "pnpm" field in package.json is no longer read by pnpm). With the overrides dropped, the config no longer matches the lockfile andpnpm install --frozen-lockfilefails hard withERR_PNPM_LOCKFILE_CONFIG_MISMATCH, so thenode-checkjob would break the moment the pin is removed. Keeping a copy in both places would leave a permanent pnpm 11 warning and invite drift.overridesfrompnpm-workspace.yamltoo. A negative control confirms the file is genuinely read rather than merely tolerated: deleting it makes 10.30.3 fail with the sameERR_PNPM_LOCKFILE_CONFIG_MISMATCH.Dockerfile, nocorepack, and nopnpm/action-setup. CI already runsnix develop -c pnpm ..., so pnpm came from the flake and the pin was only overriding it. A tree-widegrepforpackageManager,corepack, and10.30now returns no matches.allowBuildsentries needed. pnpm 11 escalates ignored dependency build scripts to a hard error, but this tree has none, so the pnpm 11 install is clean.Changes
package.jsonpackageManagerand the now-ignoredpnpm.overridesblockpnpm-workspace.yamloverridespinning the patchedjs-yamlandmarkdown-itTest plan
Run against real pnpm binaries with no
--ignore-scripts, capturing each exit code separately rather than through a pipe. CI's pnpm is the flake's 11.6.0; also checked on 11.21.0 and the previously pinned 10.30.3.nix develop -c pnpm install --frozen-lockfile: rc=0 on pnpm 11.6.0, andpnpm-lock.yamlstays byte-identical, checked against the committed blob rather than a copy (git show origin/main:pnpm-lock.yaml | cmp - pnpm-lock.yamlrc=0,git diff origin/main -- pnpm-lock.yamlempty), withlockfileVersion: '9.0'unchangednix develop -c pnpm lint: rc=0, 48 files, 0 errorsnix develop -c pnpm spellcheck: 185 files, 0 issues, rc=0 (see Notes)nix flake check: rc=0, all hooks passpre-commit run --files package.json pnpm-workspace.yaml: rc=0install --frozen-lockfile: rc=0, lockfile unchangedinstall --frozen-lockfileagainst the new workspace file: rc=0. Negative control with the file removed: rc=1,ERR_PNPM_LOCKFILE_CONFIG_MISMATCHpnpm whyresolves a singlejs-yaml@4.2.0andmarkdown-it@14.2.0, and the lock holds zerojs-yaml@4.1.1/markdown-it@14.1.1entriesNotes
spellcheckrc=1 during validation was an artifact, not a regression. Validation ran in a worktree under a gitignored path, so cspell'suseGitignore: truediscovered 0 files and exited 1 (git check-ignorepoints at.gitignore:28:.claude/worktrees/). From a non-ignored path, and with--no-gitignore, it reports 185 files and 0 issues at rc=0. CI checks out at the repo root, so it is unaffected.Not verified: the Rust gates (
cargo fmt,clippy,cargo test,llvm-cov) andnix build .#oxide-codewere not run, since this change touches onlypackage.jsonandpnpm-workspace.yamland feeds no Rust input. The offline registry failure and the.npmrcopt-out attempt were not reproduced in this run; both carry over from the original investigation.