From 71e4b8626a65bca318e33f8463f07bb5ebc55a1e Mon Sep 17 00:00:00 2001 From: Cohen Robinson Date: Fri, 21 Aug 2026 23:23:28 +1000 Subject: [PATCH 1/3] fix(lock-file): refuse a refresh that drops libc discriminators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/reusable-lock-file-npm.yml | 27 ++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reusable-lock-file-npm.yml b/.github/workflows/reusable-lock-file-npm.yml index 28e0e1c..5a5c79e 100644 --- a/.github/workflows/reusable-lock-file-npm.yml +++ b/.github/workflows/reusable-lock-file-npm.yml @@ -24,10 +24,33 @@ jobs: - name: Install dependencies (no lifecycle scripts) run: npm install --ignore-scripts + # npm drops the `libc` discriminators off native-binary entries when it + # rewrites a lock file. Those keys are how it tells the glibc and musl + # builds apart, so without them an install inside a musl container can + # resolve a glibc binary and fail at runtime rather than at install time. + # + # This has to be caught here. The PR below is authored with GITHUB_TOKEN, + # and GitHub suppresses `pull_request` triggers for GITHUB_TOKEN-authored + # events — so the refresh PR gets no checks at all, and a silent + # deletion-only diff would otherwise merge on a glance. + - name: Refuse a refresh that drops libc discriminators + run: | + removed=$(git diff -U0 -- package-lock.json | grep -c '^-.*"libc"' || true) + added=$(git diff -U0 -- package-lock.json | grep -c '^+.*"libc"' || true) + if [ "$removed" -gt "$added" ]; then + echo "::error::This refresh removes $((removed - added)) libc discriminator(s) from package-lock.json." + echo "The glibc/musl split for native binaries would be lost. Not opening a PR." + git diff -U0 -- package-lock.json | grep -B2 '^-.*"libc"' || true + exit 1 + fi + echo "libc discriminators intact (removed=$removed added=$added)" + # Consumers protect `main`, so pushing the refreshed lock file directly # is rejected with GH006 on every week the lock actually changes - # (Utilified/.github#7). Open a PR instead — it also gives the refresh - # a CI run before it lands. + # (Utilified/.github#7). Open a PR instead, so a human sees the change. + # Note it does NOT get a CI run: create-pull-request authors with + # GITHUB_TOKEN, and GitHub suppresses `pull_request` triggers for + # GITHUB_TOKEN-authored events. The guard above is the only gate. - name: Open lock file PR uses: peter-evans/create-pull-request@5f6978faf089d4d20b00c7766989d076bb2fc7f1 # v8.1.1 with: From 289bb79d5fc08d87ecdd4ad8f9ece62d457d1095 Mon Sep 17 00:00:00 2001 From: Cohen Robinson Date: Sat, 22 Aug 2026 12:12:55 +1000 Subject: [PATCH 2/3] fix(lock-file): compare libc keys per entry and fail closed 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. --- .github/workflows/reusable-lock-file-npm.yml | 70 ++++++++++++++++++-- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/.github/workflows/reusable-lock-file-npm.yml b/.github/workflows/reusable-lock-file-npm.yml index 5a5c79e..ddbe91d 100644 --- a/.github/workflows/reusable-lock-file-npm.yml +++ b/.github/workflows/reusable-lock-file-npm.yml @@ -35,15 +35,71 @@ jobs: # deletion-only diff would otherwise merge on a glance. - name: Refuse a refresh that drops libc discriminators run: | - removed=$(git diff -U0 -- package-lock.json | grep -c '^-.*"libc"' || true) - added=$(git diff -U0 -- package-lock.json | grep -c '^+.*"libc"' || true) - if [ "$removed" -gt "$added" ]; then - echo "::error::This refresh removes $((removed - added)) libc discriminator(s) from package-lock.json." - echo "The glibc/musl split for native binaries would be lost. Not opening a PR." - git diff -U0 -- package-lock.json | grep -B2 '^-.*"libc"' || true + set -euo pipefail + + lock=package-lock.json + + # Fail closed. "I found no problem" and "I inspected nothing" must not + # print the same line, so anything that leaves the guard with nothing + # to look at is an error, not a pass. + if [ ! -f "$lock" ]; then + echo "::error::No $lock at the repository root, so the libc guard could not inspect this refresh. Not opening a PR." + exit 1 + fi + if ! git cat-file -e "HEAD:$lock" 2>/dev/null; then + echo "::error::$lock is not tracked at the repository root in HEAD, so the libc guard has no baseline to compare against. Not opening a PR." + exit 1 + fi + + if git diff --quiet -- "$lock"; then + echo "$lock is unchanged by this refresh - nothing to verify, and nothing to open a PR for." + exit 0 + fi + + work=$(mktemp -d) + trap 'rm -rf "$work"' EXIT + git show "HEAD:$lock" > "$work/before.json" + + has_packages() { jq -e '(.packages? | type) == "object"' "$1" >/dev/null 2>&1; } + if ! has_packages "$work/before.json"; then + echo "::error::$lock in HEAD has no \"packages\" map (unreadable or an unexpected lock file version), so there is nothing the libc guard can compare. Not opening a PR." exit 1 fi - echo "libc discriminators intact (removed=$removed added=$added)" + if ! has_packages "$lock"; then + echo "::error::The refreshed $lock has no \"packages\" map (unreadable or an unexpected lock file version). Not opening a PR." + exit 1 + fi + + # Compare per-entry identity, not a net line count. Counting treats + # libc keys as fungible, so a refresh that strips the key off one + # entry while adding a new native package carrying its own cancels to + # zero - which is precisely the churn this guard exists to catch. + libc_paths() { + jq -r '.packages | to_entries[] | select(.value.libc != null) | .key' "$1" | LC_ALL=C sort + } + libc_paths "$work/before.json" > "$work/libc-before" + libc_paths "$lock" > "$work/libc-after" + jq -r '.packages | keys[]' "$lock" | LC_ALL=C sort > "$work/entries-after" + + # Lost the key but kept the entry = the discriminator was stripped. + # Lost the key along with the entry = the dependency is simply gone, + # which is a legitimate refresh and must not trip the guard. + comm -23 "$work/libc-before" "$work/libc-after" > "$work/untagged" + comm -12 "$work/untagged" "$work/entries-after" > "$work/stripped" + + if [ -s "$work/stripped" ]; then + echo "::error::This refresh strips the libc discriminator from $(wc -l < "$work/stripped" | tr -d ' ') package-lock.json entry/entries that still exist." + echo "The glibc/musl split for native binaries would be lost. Not opening a PR. Affected entries:" + sed 's/^/ - /' "$work/stripped" + exit 1 + fi + + tagged=$(wc -l < "$work/libc-before" | tr -d ' ') + if [ "$tagged" -eq 0 ]; then + echo "No entry in $lock carried a libc discriminator before this refresh, so none could be lost." + else + echo "libc discriminators intact: all $tagged tagged entry/entries either kept theirs or were removed outright." + fi # Consumers protect `main`, so pushing the refreshed lock file directly # is rejected with GH006 on every week the lock actually changes From 425b5fb58cead9a27aa3383ffb7f4f685987111a Mon Sep 17 00:00:00 2001 From: Cohen Robinson Date: Sat, 22 Aug 2026 12:13:01 +1000 Subject: [PATCH 3/3] test(lock-file): cover the libc guard's regression cases 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. --- .github/workflows/self-test.yml | 43 +++++++++ README.md | 11 +++ tests/lock-file-libc-guard.test.sh | 144 +++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 .github/workflows/self-test.yml create mode 100755 tests/lock-file-libc-guard.test.sh diff --git a/.github/workflows/self-test.yml b/.github/workflows/self-test.yml new file mode 100644 index 0000000..9c739c7 --- /dev/null +++ b/.github/workflows/self-test.yml @@ -0,0 +1,43 @@ +name: Self-tests + +# The reusable workflows here are consumed org-wide, and their `run:` blocks get +# no exercise in this repo otherwise. `tests/` holds shell harnesses that pull a +# step's script straight out of its YAML and run it against scratch fixtures. +on: + pull_request: + paths: + - ".github/workflows/**" + - "tests/**" + push: + branches: [main] + paths: + - ".github/workflows/**" + - "tests/**" + workflow_dispatch: + +permissions: + contents: read + +jobs: + shell-tests: + runs-on: ubuntu-latest + steps: + - uses: step-security/harden-runner@002fdce3c6a235733a90a27c80493a3241e56863 # v2.12.1 + with: + egress-policy: audit + + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Run shell test harnesses + run: | + set -euo pipefail + shopt -s nullglob + harnesses=(tests/*.test.sh) + if [ ${#harnesses[@]} -eq 0 ]; then + echo "::error::No harnesses found under tests/ - this job would otherwise pass having run nothing." + exit 1 + fi + for t in "${harnesses[@]}"; do + echo "==> $t" + bash "$t" + done diff --git a/README.md b/README.md index 71eb27f..177427f 100644 --- a/README.md +++ b/README.md @@ -70,6 +70,17 @@ jobs: python-version: "3.12" ``` +## Tests + +`tests/*.test.sh` exercise the non-trivial `run:` blocks in the reusable +workflows. A harness extracts the step's script out of its YAML and runs it +against scratch git repos, so there is no second copy to drift. `self-test.yml` +runs them on every PR touching `.github/workflows/` or `tests/`. + +```sh +bash tests/lock-file-libc-guard.test.sh +``` + ## Not in this repo Two workflows stay per-repo because their content is project-specific: diff --git a/tests/lock-file-libc-guard.test.sh b/tests/lock-file-libc-guard.test.sh new file mode 100755 index 0000000..a3eb08c --- /dev/null +++ b/tests/lock-file-libc-guard.test.sh @@ -0,0 +1,144 @@ +#!/usr/bin/env bash +# Exercises the "Refuse a refresh that drops libc discriminators" guard in +# .github/workflows/reusable-lock-file-npm.yml. +# +# The guard has to live inline in the workflow: a reusable workflow's steps run +# against the *caller's* checkout, so a script file in this repo is not on disk +# at runtime. To avoid a second copy drifting from the real one, this harness +# extracts the step's `run:` block straight out of the YAML and executes that. +# +# Usage: tests/lock-file-libc-guard.test.sh +set -uo pipefail + +repo_root=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) +workflow="$repo_root/.github/workflows/reusable-lock-file-npm.yml" +step_name="Refuse a refresh that drops libc discriminators" + +sandbox=$(mktemp -d) +trap 'rm -rf "$sandbox"' EXIT + +guard="$sandbox/guard.sh" + +# Pull the step's shell body out of the YAML, dedented. Deliberately dependency +# free (no PyYAML, no yq) so this runs anywhere bash and jq do. +awk -v want=" - name: $step_name" ' + !in_step { if ($0 == want) in_step = 1; next } + !in_run { + if ($0 == " run: |") { in_run = 1; next } + if ($0 ~ /^ - /) exit 1 + next + } + { + if ($0 ~ /^[[:space:]]*$/) { print ""; next } + if ($0 !~ /^ /) exit 0 + print substr($0, 11) + } +' "$workflow" > "$guard" || { echo "FATAL: no 'run:' block for step \"$step_name\" in $workflow"; exit 1; } + +[ -s "$guard" ] || { echo "FATAL: could not extract the guard from $workflow"; exit 1; } +chmod +x "$guard" + +failures=0 + +# make_lock +make_lock() { mkdir -p "$(dirname "$1")"; printf '%s\n' "$2" > "$1"; } + +# scratch — a git repo whose HEAD holds the "before" lock file +scratch() { + local dir="$sandbox/$1" + mkdir -p "$dir" + git -C "$dir" init -q -b main + git -C "$dir" config user.email t@example.com + git -C "$dir" config user.name Test + printf '%s\n' "$dir" +} + +commit_all() { git -C "$1" add -A && git -C "$1" commit -q -m "$2"; } + +# expect +expect() { + local name="$1" want="$2" dir="$3" out status + out=$(cd "$dir" && bash "$guard" 2>&1); status=$? + local got=pass; [ "$status" -ne 0 ] && got=fail + printf '%s\n' "--- $name" + printf '%s\n' "$out" | sed 's/^/ /' + if [ "$got" = "$want" ]; then + printf ' => %s (exit=%d) OK\n\n' "$got" "$status" + else + printf ' => %s (exit=%d) EXPECTED %s ** FAILURE **\n\n' "$got" "$status" "$want" + failures=$((failures + 1)) + fi +} + +# A lock file entry helper: with and without the libc discriminator. +NATIVE_WITH='{"lockfileVersion":3,"packages":{"":{"name":"app"}, + "node_modules/@img/sharp-linux-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"],"libc":["glibc"]}, + "node_modules/@img/sharp-linuxmusl-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"],"libc":["musl"]}, + "node_modules/lodash":{"version":"4.17.21"}}}' +NATIVE_LOST_ONE='{"lockfileVersion":3,"packages":{"":{"name":"app"}, + "node_modules/@img/sharp-linux-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"]}, + "node_modules/@img/sharp-linuxmusl-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"],"libc":["musl"]}, + "node_modules/lodash":{"version":"4.17.21"}}}' +NATIVE_LOST_ONE_GAINED_ONE='{"lockfileVersion":3,"packages":{"":{"name":"app"}, + "node_modules/@img/sharp-linux-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"]}, + "node_modules/@img/sharp-linuxmusl-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"],"libc":["musl"]}, + "node_modules/@rollup/rollup-linux-x64-gnu":{"version":"4.9.0","os":["linux"],"cpu":["x64"],"libc":["glibc"]}, + "node_modules/lodash":{"version":"4.17.21"}}}' +NATIVE_BUMPED='{"lockfileVersion":3,"packages":{"":{"name":"app"}, + "node_modules/@img/sharp-linux-x64":{"version":"0.33.5","os":["linux"],"cpu":["x64"],"libc":["glibc"]}, + "node_modules/@img/sharp-linuxmusl-x64":{"version":"0.33.5","os":["linux"],"cpu":["x64"],"libc":["musl"]}, + "node_modules/lodash":{"version":"4.17.21"}}}' +NATIVE_DROPPED_DEP='{"lockfileVersion":3,"packages":{"":{"name":"app"}, + "node_modules/@img/sharp-linuxmusl-x64":{"version":"0.33.0","os":["linux"],"cpu":["x64"],"libc":["musl"]}, + "node_modules/lodash":{"version":"4.17.21"}}}' + +# ---------------------------------------------------------------- CASE A +# The case the guard was written for: an entry loses its libc key outright. +d=$(scratch case-a) +make_lock "$d/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +make_lock "$d/package-lock.json" "$NATIVE_LOST_ONE" +expect "CASE A: existing entry loses libc, nothing gains one" fail "$d" + +# ---------------------------------------------------------------- CASE B +# Churn: one entry loses its key while a newly added native package brings its +# own. A net count of removed-vs-added lines cancels to zero here. +d=$(scratch case-b) +make_lock "$d/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +make_lock "$d/package-lock.json" "$NATIVE_LOST_ONE_GAINED_ONE" +expect "CASE B: entry loses libc (-1) while a new native pkg gains one (+1)" fail "$d" + +# ---------------------------------------------------------------- CASE C +# Nothing to inspect: no lock file at the repository root. A guard that +# inspected nothing must not report success. +d=$(scratch case-c) +make_lock "$d/apps/web/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +make_lock "$d/apps/web/package-lock.json" "$NATIVE_LOST_ONE" +expect "CASE C: no package-lock.json at the repo root (inspects nothing)" fail "$d" + +# ---------------------------------------------------------------- CASE D +# Control: a clean refresh that bumps versions and keeps every libc key. +d=$(scratch case-d) +make_lock "$d/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +make_lock "$d/package-lock.json" "$NATIVE_BUMPED" +expect "CASE D: version bump, every libc key retained" pass "$d" + +# ---------------------------------------------------------------- CASE E +# Control: the dependency is genuinely gone, key and entry together. Removing a +# package must not be mistaken for stripping its discriminator. +d=$(scratch case-e) +make_lock "$d/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +make_lock "$d/package-lock.json" "$NATIVE_DROPPED_DEP" +expect "CASE E: native dependency removed entirely (entry and key)" pass "$d" + +# ---------------------------------------------------------------- CASE F +# Control: the refresh was a no-op. Must be reported as unchanged, not as +# verified-clean. +d=$(scratch case-f) +make_lock "$d/package-lock.json" "$NATIVE_WITH"; commit_all "$d" before +expect "CASE F: refresh left the lock file untouched" pass "$d" + +if [ "$failures" -ne 0 ]; then + echo "$failures case(s) behaved incorrectly." + exit 1 +fi +echo "All cases behaved as expected."