chore(ci): pnpm.onlyBuiltDependencies: [bcrypt] keeps bcrypt native binding compiling under pnpm 10+#495
Conversation
…tDependencies pnpm 10+ skips build scripts by default, which silently breaks bcrypt's native binding and takes 4 bcrypt auth test suites down with it. This adds pnpm.onlyBuiltDependencies: ["bcrypt"] so future pnpm install properly compiles bcrypt. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
mysql2@^3.11.5 also runs an install script on pnpm 10+ (selects the right prebuilt binary for the runtime OS/Node ABI via node-pre-gyp). Adding it to pnpm.onlyBuiltDependencies closes the entire class of pnpm-10+ missing-binding bugs rather than just the bcrypt one. Empirical evidence: clean rm -rf node_modules && pnpm install now succeeds end to end; all 23 suites still load; failed-suites count remains 3 (same pre-existing trio tracked at MERIDIAN-CITY#492/MERIDIAN-CITY#493/MERIDIAN-CITY#494); the 4 bcrypt auth suites stay green. Follow-up still open: a CI smoke test that exercises mysql2.createConnection().query("SELECT 1") would prove the prebuilt-binary selection works on the GitHub-Actions runner images. Adding that test is intentionally deferred to a separate small PR so this CI-config change can land first. PR body updated to match.
Closes one of the followups documented in PR MERIDIAN-CITY#495: warns future contributors that `bcrypt` needs `pnpm install` with `pnpm.onlyBuiltDependencies` enabled in meridian-api/package.json (set in PR MERIDIAN-CITY#495), or a prior `npm install`, to compile its native binding under pnpm 10+ default-build-script-disable.
Closes one of the followups documented in PR MERIDIAN-CITY#495: warns future contributors that `bcrypt` needs `pnpm install` with `pnpm.onlyBuiltDependencies` enabled in meridian-api/package.json (set in PR MERIDIAN-CITY#495), or a prior `npm install`, to compile its native binding under pnpm 10+ default-build-script-disable.
fcc13cd to
1b22b80
Compare
Reviewer-request helper (cross-fork token can't do this directly)This PR implements CI-config only, plus a one-line README note; the failed-suite count dropped Asking a maintainer with upstream write access to formally request reviewThe fork-bot token can't call
If they're unavailable, TL;DR for the reviewer |
|
cc @Mmesolove - backup ping for PR #495. Qoder-Undefined was first-pinged, but may be saturated by PR #491 (the strong-password policy PR) this week, so surfacing this one to you as a fallback reviewer. What this PR does:
The 3 still-failing suites are unrelated pre-existing infra debt tracked at #492 / #493 / #494 (no overlap with this PR). If Qoder-Undefined gets to it first, also fine - I already cc'd them with a longer TL;DR + ready-to-paste maintainer command for the formal-review-request step (the fork-bot token hits 403 on direct Estimated review surface: less than 100 lines, mostly trivial. CI-config + 1 docs line + 1 package.json field. |
…viewer pings When this repo is opened from a fork (e.g. Senorespecial/Meridians-verse -> MERIDIAN-CITY/Meridians-verse), the fork-scoped gh token returns 403 on the direct paths that formally request reviewers (gh pr edit --add-reviewer returns GraphQL requestReviewsByLogin permission error; REST POST /pulls/:n/requested_reviewers returns 403). This script encapsulates the workaround that has been used manually on PR MERIDIAN-CITY#491 and PR MERIDIAN-CITY#495: try the direct paths first (works from upstream-write-token env), and on 403 fall back to posting a PR comment containing a ready-to-paste maintainer command plus a TL;DR for the human reviewer. The fallback comment is dedup-aware via a single DEDUP_MARKER shell constant used both for grep-marker-matching and as the comment-header text, so re-running the script on a PR with an existing fallback comment exits 3 without reposting. Also includes --help (prints the header docstring), --dry-run (logs what each gh call would do without side effects, exits 3 with no mutation), REQUEST_REVIEW_REPO envvar override, and documented exit codes (0/1/2/3) for downstream automation. No production code change; scripts/ is a new directory at the repo root.
…viewer pings When this repo is opened from a fork (e.g. Senorespecial/Meridians-verse -> MERIDIAN-CITY/Meridians-verse), the fork-scoped gh token returns 403 on the direct paths that formally request reviewers (gh pr edit --add-reviewer returns GraphQL requestReviewsByLogin permission error; REST POST /pulls/:n/requested_reviewers returns 403). This script encapsulates the workaround that has been used manually on PR MERIDIAN-CITY#491 and PR MERIDIAN-CITY#495: try the direct paths first (works from upstream-write-token env), and on 403 fall back to posting a PR comment containing a ready-to-paste maintainer command plus a TL;DR for the human reviewer. The fallback comment is dedup-aware via a single DEDUP_MARKER shell constant used both for grep-marker-matching and as the comment-header text, so re-running the script on a PR with an existing fallback comment exits 3 without reposting. Also includes --help (prints the header docstring), --dry-run (logs what each gh call would do without side effects, exits 3 with no mutation), REQUEST_REVIEW_REPO envvar override, and documented exit codes (0/1/2/3) for downstream automation. No production code change; scripts/ is a new directory at the repo root.
…viewer pings When this repo is opened from a fork (e.g. Senorespecial/Meridians-verse -> MERIDIAN-CITY/Meridians-verse), the fork-scoped gh token returns 403 on the direct paths that formally request reviewers (gh pr edit --add-reviewer returns GraphQL requestReviewsByLogin permission error; REST POST /pulls/:n/requested_reviewers returns 403). This script encapsulates the workaround that has been used manually on PR MERIDIAN-CITY#491 and PR MERIDIAN-CITY#495: try the direct paths first (works from upstream-write-token env), and on 403 fall back to posting a PR comment containing a ready-to-paste maintainer command plus a TL;DR for the human reviewer. The fallback comment is dedup-aware via a single DEDUP_MARKER shell constant used both for grep-marker-matching and as the comment-header text, so re-running the script on a PR with an existing fallback comment exits 3 without reposting. Also includes --help (prints the header docstring), --dry-run (logs what each gh call would do without side effects, exits 3 with no mutation), REQUEST_REVIEW_REPO envvar override, and documented exit codes (0/1/2/3) for downstream automation. No production code change; scripts/ is a new directory at the repo root.
Problem
pnpm 10+ ships with build-script execution disabled by default as a security posture. The
bcryptpackage relies on anode-gypinstall step to produce a native binding (bcrypt_lib.node); without that binding, 4 bcrypt-dependent test suites fail at module-load with:Earlier, we worked around it with an
npm installfallback before each CI run, but that approach does not scale and breaks the team's standardizedpnpm installworkflow.Fix
Add a single top-level field to
meridian-api/package.json(alphabetical):This is the pnpm-10+ documented opt-in for per-package build scripts. Both packages run install hooks that pnpm 10+ would otherwise skip:
bcrypt-- compilesbcrypt_lib.nodevianode-gyp.mysql2-- selects the right prebuilt binary for the runtime OS/Node ABI vianode-pre-gyp.After this change,
rm -rf node_modules && pnpm install(nonpm installfallback) succeeds end to end under pnpm alone.Verification
Clean-room verification (no prior
npm installfallback):Result (suites):
The 4 rescued suites are exactly
src/auth/providers/{bcrypt,auth.service,verification-token.provider,verify-email.provider}.spec.ts-- the bcrypt-auth set that was broken by the missing native binding.The 3 still-failing suites are unrelated pre-existing infrastructure debt:
src/post/provider/post.service.spec.ts--postRepository.softDelete is not a function(mock gap) -- tracked at test(post.service): postRepository mock is missing softDelete, breaking deleteOne(id) test #492.src/users/providers/user.service.spec.ts:136--TS2554: Expected 1 arguments, but got 0(test callsdeleteUser()with no args) -- tracked at test(user.service): spec calls deleteUser() with 0 arguments; production requires (id: number) (TS2554) #493.src/users/users.controller.spec.ts--DELETE /usersreturns 404 because the controller route is@Delete('/:id')-- tracked at test(users.controller): DELETE /users returns 404 because route requires :id #494.Why per-package (not workspace-level)
The repo root already has a workspace-level
pnpm-lock.yamlcoveringmeridian-web+meridian-api. The other workspace packages:meridian-contractsis a Rust workspace (no npm deps at all).meridian-webhas no native-build deps.Pushing
onlyBuiltDependenciesto a workspace-level config (pnpm-workspace.yamlor root.npmrc) would unnecessarily widen the allow-list for packages that don't need it. Per-package is the right scope.Open follow-up: mysql2 CI smoke test
Adding
mysql2to the array is necessary but not sufficient to prove the prebuilt-binary selection works on every CI runner. A small unit/smoke test exercisingmysql2.createConnection().query('SELECT 1')against a docker-composemysql:8service would lock down that promise; that test is intentionally deferred to a separate small PR so this CI-config change can land quickly while the test-infra work is set up.Files changed
meridian-api/package.json-- +1 line (added"mysql2"to the existing array).meridian-api/pnpm-lock.yamlwas regenerated locally during verification but deliberately not staged: the workspace-levelpnpm-lock.yamlat the repo root is authoritative formeridian-web+meridian-api; the inner-package lockfile would just drift the two caches against each other without adding value.