Skip to content

release-age: keep system npm out of the test sandbox - #189

Merged
DJRHails merged 1 commit into
mainfrom
release-age-test-path
Oct 9, 2026
Merged

DJRHails merged 1 commit into
mainfrom
release-age-test-path

Conversation

@DJRHails

@DJRHails DJRHails commented Oct 9, 2026

Copy link
Copy Markdown
Owner

What

Follow-up from the review of #187 (already merged). tests/release-age.test.sh claimed its sandbox let only the stub npm/pnpm answer, but run_setup put /usr/bin:/bin on PATH. On a host with a system npm in /usr/bin (NodeSource, or Debian's npm package), the real npm answered the no-npm case: setup reported "npm 10.9.9 predates min-release-age" instead of "npm not installed", so the suite exited 1. The suite runs as an unconditional lefthook pre-commit hook (release-age-tests, no glob), so on such a host every commit to this repo was blocked. The bug dates from f7a6cc7 (#185).

The sandbox now gets a symlinked copy of /usr/bin and /bin with npm, npx, pnpm, pnpx and corepack left out, which makes the comment's guarantee true.

Verification

  • On a host with /usr/bin/npm (10.9.9) and /usr/bin/pnpm: before the change, bash tests/release-age.test.sh gave 1 failed (no-npm-message). After it, all 54 checks pass.
  • shellcheck is clean, shfmt -i 2 -d shows no diff, and the lefthook pre-commit suite passed on commit.

via gantry

- F1 (P2, fixed): release-age.test.sh put /usr/bin on run_setup's PATH, so
  on a host with a system npm (NodeSource, Debian's npm) the real npm
  answered the no-npm case and the suite failed; it runs unconditionally
  as a pre-commit hook, so every commit on such a host was blocked. The
  sandbox now gets a filtered system bin dir without npm/npx/pnpm/pnpx/
  corepack. Origin: f7a6cc7 (#185).
- F2 (P3, left for the author): README presents npm config set -g as the
  way to edit the tracked npmrc, but npm rewrites it without its comments.
@DJRHails

DJRHails commented Oct 9, 2026

Copy link
Copy Markdown
Owner Author

Review Summary

No P1–P3 findings. The PR makes the test file's comment true: run_setup's PATH is now the stubs plus a symlink copy of /usr/bin and /bin without npm/npx/pnpm/pnpx/corepack. /usr/bin is linked first and existing names are skipped, so the order matches the old /usr/bin:/bin PATH. On merged-usr hosts, the duplicate /bin entries are skipped.

# Severity Finding Resolution
— P4 If /usr/bin holds a dangling symlink (for example a broken /etc/alternatives target) on a merged-usr host, [ -e ] is false for the copied link, so the /bin pass prints ln: … File exists to stderr. The suite is set -u only, so this is noise, not a failure. [ -e x ] || [ -L x ] would silence it. Observation only

Verified: I ran this on a host with /usr/bin/npm and /usr/bin/pnpm. The base version of tests/release-age.test.sh, run in place, gave 1 failed (no-npm-message: "npm 10.9.9 predates min-release-age"). The PR head gave all passed in 1.5 s. shellcheck is clean and shfmt -i 2 -d shows no diff. setup.sh only checks npm/pnpm with platform::command_exists, so leaving out those names is enough.
Fix commit: none pushed
Reviewed head: d0a5734

Verdict: approve

via gantry

@DJRHails
DJRHails merged commit cd6d75a into main Oct 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant