fix(lock-file): refuse a refresh that drops libc discriminators - #11
Merged
Conversation
The weekly refresh has been opening deletion-only PRs that strip every `libc` key from the native-binary entries — platform-infra#200 removed six, ums-web#758 removed four, both +0/-N. npm does this whenever it rewrites a lock file. Those keys are how npm tells the glibc and musl builds of a native package apart. Without them an install inside a musl container can resolve a glibc binary, which fails at runtime rather than at install time. Nothing was catching it. create-pull-request authors with GITHUB_TOKEN, and GitHub suppresses `pull_request` triggers for GITHUB_TOKEN-authored events, so these PRs get no checks at all — the existing comment claiming the PR "gives the refresh a CI run before it lands" is not true in practice. A silent deletion-only diff is exactly the shape that merges on a glance. Adds a guard between the install and the PR: if the refresh removes more libc keys than it adds, the job fails and no PR is opened. Comparing net rather than absolute so a diff that merely rewrites those lines does not false-positive. Also corrects the misleading comment about CI. Verified both directions against a scratch repo: a stripped lock file fails with a count and the offending context; a version bump that rewrites the same lines passes (removed=2 added=2). Claude-Session: https://claude.ai/code/session_01VLwoNAdLUEAxVL4AymEuhJ
The guard compared a net removed-vs-added count of libc lines, which treats the keys as fungible: a refresh that strips the discriminator off an existing entry while adding a new native package carrying its own nets to zero and passes. That is the churn case the guard exists to catch. Compare the set of package paths carrying a libc key instead. A path that had one, still exists, and no longer has one is a strip; a path that lost its key along with its entry is just a removed dependency, which the count-based check wrongly failed. Also fail closed. Both counts landed on 0 when the guard inspected nothing at all - no lock file at the repo root, no baseline in HEAD, an unreadable file - and it printed the same success line as a clean refresh. Each of those is now an error, and an unchanged lock file is reported as unchanged rather than as verified. The jq lookup drops the unanchored substring match and names the offending entries in the failure output.
The reusable workflows' run: blocks get no exercise in this repo. The harness extracts the guard's script straight out of its YAML and runs it against scratch git repos, so there is no second copy to drift out of step. Covers both defects that shipped - net-count cancellation and the fail-open no-op - plus the false positive on a genuinely removed dependency, and pins the unchanged-vs-verified distinction in the output.
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 problem
The weekly refresh has been opening deletion-only PRs that strip every
libckey from native-binary entries:platform-infra#200—+0/-18, sixlibckeys removedums-web#758—+0/-12, four removednpm does this whenever it rewrites a lock file. Those keys are how it tells the glibc and musl builds of a native package apart — without them, an install inside a musl container can resolve a glibc binary, which then fails at runtime rather than at install time.
I hit this twice by hand earlier this week on
platform-infraandums-portaland had to rebuild the lock file frommainwith only the intended entry applied. Both PRs above are now closed.Why nothing caught it
peter-evans/create-pull-requestauthors withGITHUB_TOKEN, and GitHub suppressespull_requestworkflow triggers for GITHUB_TOKEN-authored events. These PRs get no checks at all.The existing comment in this workflow says opening a PR "also gives the refresh a CI run before it lands". That isn't true in practice, and it's the reason a silent deletion-only diff has been reaching a merge button unguarded. Corrected here too.
The fix
A guard between the install and the PR. If the refresh removes more
libckeys than it adds, the job fails and no PR is opened.It compares net rather than absolute, so a diff that merely rewrites those lines — reindentation, a version bump touching the same entry — doesn't false-positive.
Verified both directions
Against a scratch repo:
libcstrippedremoved=2 added=2)Note
This only stops the bad refresh landing; it doesn't stop npm doing it. When the guard fires, someone still has to produce a clean refresh by hand — take
main's lock file and apply only the intended entries, which is what I did on the two repos above. Pinning npm to a version that preserves the keys would be the real fix, if one exists that also satisfies the.nvmrcpins across consumers.