diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c454d19c9..ec0226cd6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -217,9 +217,15 @@ jobs: cache: 'pnpm' registry-url: 'https://registry.npmjs.org' - # semantic-release publishes via @semantic-release/npm, which shells out - # to `npm publish`. OIDC trusted publishing requires npm >= 11.5.1, so - # pin a known-good npm even though dependencies are managed by pnpm. + # bulma-ui, create-bestax and bestax-mcp publish via + # @semantic-release/npm, which shells out to `npm publish`. OIDC trusted + # publishing requires npm >= 11.5.1, so pin a known-good npm even though + # dependencies are managed by pnpm. + # + # bestax-migrate is the exception and does not need this: it publishes + # with `pnpm publish` (#436), which carries its own OIDC exchange, because + # `npm publish` does not resolve the `workspace:` protocol it keeps in + # devDependencies (#412). Its release step below is otherwise identical. - name: Update npm for OIDC trusted publishing run: npm install -g npm@latest diff --git a/.gitignore b/.gitignore index aaa4bb6a7..f561d7f85 100644 --- a/.gitignore +++ b/.gitignore @@ -72,7 +72,6 @@ web_modules/ # Output of 'npm pack' *.tgz bulma-ui/CLAUDE.md.bak -bestax-migrate/package.json.pack-backup # Yarn Integrity file .yarn-integrity @@ -169,3 +168,8 @@ test-apps/ test-app-*/ create-bestax/e2e/test-results/ create-bestax/e2e/playwright-report/ + +# Written only by the pack-manifest resolver deleted in #436. Kept so a stale +# backup left on disk by a pre-#436 checkout cannot be committed by a `git add +# -A` after rebasing. Safe to drop once no working copy predates that change. +bestax-migrate/package.json.pack-backup diff --git a/CLAUDE.md b/CLAUDE.md index bc560a4cb..5775a08f5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -89,9 +89,18 @@ Full versioning details (breaking-change footers, tag formats): `VERSIONING.md`. become permanent (#391). A blocking audit gate plus the cooldown means a fresh advisory can red every open PR — CONTRIBUTING.md has the runbook. - Isolated node linker: undeclared (phantom) dependencies fail — declare everything you import. -- Published packages must not ship a `workspace:` or `catalog:` specifier — `npm publish` - (what semantic-release runs) resolves neither, and the tarball becomes uninstallable - (#412). `check:conformance --only=publishable-manifests` enforces this. +- **How a package publishes decides what its manifest may contain.** `npm publish` resolves + no pack-time protocol at all, so a package published that way must not ship one — the + tarball becomes uninstallable (#412). bestax-migrate hands its publish step to + `pnpm publish` instead (#436), which buys it a **narrow** exemption: + `workspace:`/`catalog:` in **devDependencies** only. `jsr:` becomes an aliased + `npm:@jsr/…` specifier and `link:`/`portal:`/`file:` are not rewritten at all, so those + four are a violation in **any** section, exemption or not. `workspace:`/`catalog:` are + additionally a violation in a section consumers resolve, since pnpm resolving them does + not stop every consumer being made to install the dependency. Which packages publish with pnpm + is **declared** in `check:conformance` rather than inferred from their release config — + inferring it meant parsing semantic-release's config format, which was wrong four times, + and every miss granted the exemption. ## Workflow diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4a21af8b1..ac52d14ce 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -255,7 +255,7 @@ no `npm publish`, no tag, no GitHub release. > **Safe to run; never publishes:** everything above. The only things that actually publish are > `pnpm exec semantic-release` **without** `--dry-run` (CI-only, on merge to `main`) and a manual -> `npm publish` — neither of which is in this list. +> `npm publish` / `pnpm publish` — none of which is in this list. --- diff --git a/SECURITY.md b/SECURITY.md index 381af57f9..0a3c56569 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -43,9 +43,12 @@ Measures active in this repository and its release pipeline: reviewed lockfile resolves. (The React 18/19 compatibility matrix is the one deliberate exception: it re-resolves to pin the requested React major for testing, and never publishes.) -- **npm provenance** — all four published packages set - `publishConfig.provenance`, so every release carries a signed attestation - linking the tarball to the exact commit and CI run that built it. +- **npm provenance** — every release carries a signed attestation linking the + tarball to the exact commit and CI run that built it. Three packages request + it with `publishConfig.provenance`; bestax-migrate publishes with + `pnpm publish`, which does not read that field, so it passes `--provenance` + on the command instead and carries no `publishConfig.provenance` at all + (having one there would imply the flag was redundant). - **OIDC trusted publishing** — releases authenticate to npm with short-lived OIDC tokens minted per run; there is no long-lived `NPM_TOKEN` to steal. diff --git a/VERSIONING.md b/VERSIONING.md index a845a028e..833f80097 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -57,9 +57,16 @@ Each package tags and logs its own releases: On merge to `main`, CI (`.github/workflows/ci.yml`) runs semantic-release in each package: 1. Each package analyzes the commits since **its own** last tag against its `releaseRules`. -2. If a release is due: version bump, `CHANGELOG.md` update, npm publish (OIDC trusted +2. If a release is due: version bump, `CHANGELOG.md` update, publish to npm (OIDC trusted publishing — no `NPM_TOKEN`), a signed `chore(release): X.Y.Z [skip ci]` commit, git tag, and GitHub release. + - Three packages publish via `@semantic-release/npm`, which shells out to `npm publish`. + **bestax-migrate publishes with `pnpm publish`** (`@semantic-release/exec`), because it + keeps a `workspace:` devDependency and `npm publish` ships that protocol verbatim — + which is how 1.0.0 went out uninstallable (#412, #436). + - Note the ordering, because it decides what a failed publish costs: semantic-release runs + **every** `prepare` step — including the release commit and tag — before **any** `publish` + step. A publish that fails leaves the commit and tag behind, and that version is spent. 3. A push may release any subset of the packages — they never bump each other. `main` is ruleset-protected, so the release commit and tag are pushed by a dedicated diff --git a/bestax-migrate/CLAUDE.md b/bestax-migrate/CLAUDE.md index 39496c0ea..5138b4b43 100644 --- a/bestax-migrate/CLAUDE.md +++ b/bestax-migrate/CLAUDE.md @@ -62,19 +62,93 @@ each source library registers in `src/sources/registry.ts`; the first is ## Releases Independent semantic-release, keyed off the `bestax-migrate` commit scope -(`release.config.js`, tag `bestax-migrate@x.y.z`). Publishing goes through -`npm publish`, which — unlike `pnpm publish` — does **not** resolve pnpm's -`workspace:` protocol, so `workspace:^` shipped verbatim in 1.0.0 and made the -package uninstallable (#412). Two rules follow: `@allxsmith/bestax-bulma` stays -a **devDependency** (it is only the typecheck target for the e2e, never -imported at runtime — consumers of a codemod CLI must not be made to install -the component library), and `scripts/pack-manifest.mjs` resolves any remaining -`workspace:` specifier during `prepack`/`postpack`. `pnpm check:conformance ---only=publishable-manifests` enforces the protocol half of both: no -`workspace:`/`catalog:` specifier in the sections consumers resolve, and one -left in `devDependencies` only with the pack hooks present. It does **not** -check which section `@allxsmith/bestax-bulma` sits in — re-adding it as a -plain-semver runtime dependency passes CI, so that one is on review. The skill lives at repo-root +(`release.config.js`, tag `bestax-migrate@x.y.z`). + +**This is the one package that publishes with `pnpm publish`, not `npm publish` +(#436).** `npm publish` does not resolve pnpm's `workspace:` protocol, so +`workspace:^` shipped verbatim in 1.0.0 and made the package uninstallable +(#412). That was patched by a `prepack` hook reimplementing pnpm's rewrite, and +the reimplementation was wrong twice in one review — so the publish step now +goes to `@semantic-release/exec` running `pnpm publish`, which resolves every +pnpm specifier shape by construction. `@semantic-release/npm` stays in the chain +with `npmPublish: false` purely for its `prepare` step, which writes the version +`@semantic-release/git` then commits. + +Three things about that split are load-bearing, and none of them fails loudly: + +- **`--provenance` is required.** pnpm reads `publishConfig.registry` and + `.access` but takes `provenance` from options only. `publishConfig.provenance` + was deliberately REMOVED from this package's manifest rather than left in + place: it does nothing under pnpm, and the most likely reason anyone would + delete the flag is reading `"provenance": true` in package.json and concluding + it is redundant. Drop the flag and #411's provenance quietly stops being + produced. +- **`--embed-readme` is required.** pnpm defaults it to false where npm defaults + it to true; without it the npmjs.com page loses its README. +- **The auth pre-flight is weaker than it was.** `@semantic-release/npm` + exchanged a real OIDC token during `verifyConditions`. With `npmPublish: false` + that is off, and semantic-release finishes every `prepare` step — the release + commit and the tag — before the first `publish` step. So a failed publish + leaves both behind and spends the version. + `scripts/verify-oidc-context.mjs` runs as the exec plugin's + `verifyConditionsCmd` and checks only that an OIDC context exists; it does not + prove npm will accept the token. + +**This package must be published with `pnpm publish`, and the likely mistakes +are refused.** The +old `prepack` hook rewrote `workspace:^` for whatever was packing, so it covered a +manual publish as well as the release pipeline. Deleting it left the guarantee +living only in `release.config.js`, which the conformance rule then exempts +precisely because pnpm handles it, so the specifier had no mechanical guard at all +outside CI. The hooks now run repo-root `scripts/require-pnpm-publish.mjs` (not +`bestax-migrate/scripts/`, which still exists for `validate-corpus.mjs`), which +refuses packers it recognises as not being pnpm. Both `npm publish` and `pnpm +publish` run those hooks. + +**It keys on `npm_execpath`, not `npm_config_user_agent`, and that is not a +detail to tidy up.** The user agent is inherited: npm relays whatever it finds, +so `pnpm exec npm publish` runs the hook reporting `pnpm/…` while npm assembles +the tarball, and an agent check waves it through. `npm_execpath` is rewritten by +whichever process actually runs the script, so it names the real packer. + +**It refuses named packers, and deliberately allows unrecognised ones.** npm, +yarn, bun and friends are refused by name; anything the guard cannot place is +let through. That asymmetry is not laziness, and reversing it would be worse +than the hole it closes: pnpm's own lifecycle runner sets +`npm_execpath = process.argv[1] || process.cwd()`, so a pnpm build where +`argv[1]` is falsy reports the package **directory**. Refusing what we cannot +recognise would kill a genuine release from inside a pack hook, after +semantic-release has pushed the commit and the tag, which is the one direction +this guard must never fail in. So it is a guard against the publisher someone +actually reaches for, not a proof that only pnpm can ever pack this package. + +`pnpm check:conformance` reports a violation if either hook is missing, so the +exemption and its compensating guard cannot drift apart. (It reports the missing +hook; it does not retract the exemption, so a manifest with both problems shows +one violation for each.) + +The hook is wired to **both `prepack` and `prepublishOnly`**, and both are +required by `check:conformance`. `prepack` is the load-bearing one for `npm +pack`: `npm publish ` runs no scripts at all, so a tarball packed by +npm would otherwise be publishable with nothing left to refuse it. + +It is still a guard against the likely mistake rather than a proof. +`--ignore-scripts` skips both hooks outright (npm and pnpm each gate lifecycle +scripts on it), and a tarball packed before this existed, or packed elsewhere, +carries no guard with it. Check what a manifest will actually ship with +`pnpm -C bestax-migrate pack`. + +`@allxsmith/bestax-bulma` still stays a **devDependency** — it is only the +typecheck target for the e2e, never imported at runtime, and consumers of a +codemod CLI must not be made to install the component library. That is a policy +rule, not a protocol one, and the conformance check enforces only part of it. +The pack-time exemption this package gets is narrow: `workspace:`/`catalog:` in +**devDependencies** only. Moving the library to `dependencies` as `workspace:^` +is flagged (consumers would be made to install it), but re-adding it as a +**plain semver range** still passes CI, because that is a policy question rather +than a protocol one. That one is on review. + +The skill lives at repo-root `skills/bestax-migrate/`. It **is** bundled into create-bestax (settled in #385): the original existing-sites-only policy lost to one uniform bundle, and the skill sits idle in a fresh scaffold until legacy imports show up. The canonical roster of surfaces that diff --git a/bestax-migrate/package.json b/bestax-migrate/package.json index e037f1ec3..de05200e5 100644 --- a/bestax-migrate/package.json +++ b/bestax-migrate/package.json @@ -22,8 +22,8 @@ "format:check": "prettier --check \"src/**/*.{ts,tsx}\" \"e2e/**/*.ts\"", "clean": "rimraf dist", "release": "npx semantic-release", - "prepack": "node scripts/pack-manifest.mjs prepack", - "postpack": "node scripts/pack-manifest.mjs postpack" + "prepack": "node ../scripts/require-pnpm-publish.mjs", + "prepublishOnly": "node ../scripts/require-pnpm-publish.mjs" }, "keywords": [ "bestax", @@ -70,7 +70,6 @@ "typescript": "^6.0.3" }, "publishConfig": { - "access": "public", - "provenance": true + "access": "public" } } diff --git a/bestax-migrate/release.config.js b/bestax-migrate/release.config.js index 6754a75a3..92b399b77 100644 --- a/bestax-migrate/release.config.js +++ b/bestax-migrate/release.config.js @@ -1,3 +1,17 @@ +import path from 'node:path'; +// `sh` quotes the paths below for /bin/sh: a checkout under a directory with a +// space would otherwise split into two arguments and fail with a confusing +// "Cannot find module". It is colocated with its inverse, which the conformance +// check uses to read these commands back. +import { quote as sh } from '../scripts/lib/shell-words.mjs'; + +// Absolute, so neither exec command depends on the cwd semantic-release was +// invoked from. It works today only because ci.yml sets `working-directory`, +// and a relative `../scripts/...` would break the release the moment anything +// ran it from the repo root. +const PKG_DIR = import.meta.dirname; +const SCRIPTS = path.join(PKG_DIR, '..', 'scripts'); + export default { branches: ['main'], tagFormat: 'bestax-migrate@${version}', @@ -44,10 +58,111 @@ export default { changelogFile: 'CHANGELOG.md', }, ], + // Publishing is split deliberately (#436). This plugin keeps its `prepare` + // step — that is what writes nextRelease.version into package.json for + // @semantic-release/git to commit — but its `publish` step shells out to + // `npm publish`, which does not resolve pnpm's `workspace:` protocol and + // shipped an uninstallable 1.0.0 because of it (#412). So the publish half + // goes to `pnpm publish` below, which resolves every pnpm specifier shape + // by construction rather than by a script of ours reimplementing a subset. + // + // `npmPublish: false` also switches off this plugin's npm auth check in + // verifyConditions — see verifyConditionsCmd on the exec plugin for what + // partially replaces it, and why only partially. [ '@semantic-release/npm', { pkgRoot: '.', + npmPublish: false, + }, + ], + [ + '@semantic-release/exec', + { + // Every command below runs here, not in whatever directory + // semantic-release was started from. `pnpm publish` resolves its target + // package from the cwd, so without this a run from the repo root would + // reach the publish step (after the release commit and tag are already + // pushed) and fail on the private root package. + execCwd: PKG_DIR, + + // Guards the one failure this swap newly introduces rather than + // inherits: with npmPublish false, nothing exchanges an OIDC token + // during verifyConditions any more, and semantic-release runs every + // `prepare` step (including the release commit and tag) before any + // `publish` step. The script says what it does and does not prove. + // `${options.dryRun}` is available because exec renders its commands + // as lodash templates over the semantic-release context. Needed + // because verifyConditions is marked `dryRun: true` upstream, so this + // runs during `semantic-release --dry-run` as well, and a dry run + // publishes nothing and needs no token. + verifyConditionsCmd: + `node ${sh(path.join(SCRIPTS, 'verify-oidc-context.mjs'))}` + + ' ${options.dryRun ? "--dry-run" : ""}', + + // Every flag here is load-bearing; none is decoration. + // + // --provenance The ONLY thing turning provenance on. pnpm reads + // publishConfig.registry and .access but takes + // `provenance` from options, and it is absent from + // the whitelist that hoists publishConfig keys. The + // package.json no longer carries a + // `publishConfig.provenance` at all, precisely so + // nobody reads one and concludes this flag is + // redundant. Passing it explicitly also survives an + // OIDC response that omits provenance, since pnpm + // assigns that with `??=`. + // --embed-readme pnpm defaults this to false, npm defaults it to + // true. Without it the npmjs.com page for this + // package loses its README on the next release. + // --no-git-checks pnpm otherwise refuses to publish from a branch it + // does not recognise as the publish branch; + // semantic-release is mid-release when this runs. + // --access belt and braces, not load-bearing: pnpm falls back + // to publishConfig.access, which this package sets. + // Stated explicitly because it is the value pnpm + // errors on when generating provenance for a package + // it believes is private, so it should be visible + // next to --provenance rather than a file away. + // + // No --tag on purpose. The dist-tag would have to be derived from + // nextRelease.channel, and @semantic-release/npm does not derive it + // naively: get-channel.js maps a channel that is a valid semver RANGE + // to `release-`, because the registry rejects a dist-tag that + // parses as a range. Reimplementing that in a lodash template needs + // semver and would be a copy of upstream logic drifting out of sight, + // which is the bug class #436 exists to stop repeating. `branches` is + // ['main'], so the channel is always null and pnpm's default of + // `latest` is already right. The test asserts that `branches` has not + // changed, so adding a maintenance or prerelease branch fails CI here + // rather than silently publishing it to the stable tag. + // pnpm's own output goes to stderr so stdout carries only the JSON + // release object exec parses. Nothing is hidden by that: exec pipes + // stdout and stderr separately to the job log, so the publish output + // still appears exactly where it does today, and a failed publish + // still throws. Without it, exec's parse fails, it returns undefined, + // and the "release is available on" comment on every linked issue and + // PR shows a bare `bestax-migrate@x.y.z` instead of the npm link the + // other three packages get. + // + // The `&&` tail cannot fail the release: npm-release-info.mjs always + // exits 0, degrading to `{}` if anything goes wrong. A non-zero exit + // there would throw out of the publish step with the tarball already + // on the registry, skipping @semantic-release/github and spending the + // version, for the sake of a link in a comment. + // `|| true` and not just the script's own error handling: if node + // cannot LOAD the script (moved, renamed, a syntax error), it exits + // non-zero before that handling is ever reached, and the `&&` chain + // would then fail the publish step with the tarball already on the + // registry. The guarantee has to live in the shell, where it holds + // whatever happens to the script. + publishCmd: + 'pnpm publish --no-git-checks --provenance --embed-readme ' + + '--access public 1>&2 && { node ' + + sh(path.join(SCRIPTS, 'npm-release-info.mjs')) + + ' --dir=' + + sh(PKG_DIR) + + ' ${nextRelease.version} || true; }', }, ], [ diff --git a/bestax-migrate/scripts/pack-manifest.mjs b/bestax-migrate/scripts/pack-manifest.mjs deleted file mode 100644 index 645f021ac..000000000 --- a/bestax-migrate/scripts/pack-manifest.mjs +++ /dev/null @@ -1,235 +0,0 @@ -#!/usr/bin/env node -/** - * Resolves pnpm `workspace:` specifiers in package.json while the tarball is - * packed (issue #412). - * - * `pnpm publish` rewrites `workspace:^` to the real semver range at pack time; - * `npm publish` does NOT — and the release pipeline runs `@semantic-release/npm`, - * which shells out to `npm publish`. That shipped `bestax-migrate@1.0.0` with a - * literal `"workspace:^"` in its manifest, making it uninstallable by every - * package manager (`EUNSUPPORTEDPROTOCOL`). - * - * So do the rewrite ourselves. The repo file must come back untouched — the - * release commits package.json — so `prepack` backs it up and `postpack` - * restores it, exactly like bulma-ui/scripts/pack-pointer-files.mjs. - * - * This runs over EVERY dependency section, not just `dependencies`: a - * `workspace:` specifier is meaningless outside the workspace wherever it - * appears. The companion guard is the `publishable-manifests` sub-check in - * scripts/check-conformance.mjs, which fails CI if a published package ever - * declares a workspace dep in `dependencies` (a runtime dep the tarball would - * need) rather than `devDependencies`. - * - * Note for reviewers: the published manifest still carries the `prepack` / - * `postpack` hooks pointing here, while `files: ["dist"]` keeps this script out - * of the tarball. That is deliberate and inert — npm runs those hooks on - * pack/publish, never on install from a tarball, so the path is never followed - * by a consumer. Shipping `scripts/` to fix the dangling reference would add - * dead weight to every install, and stripping the hooks during `prepack` would - * risk npm not running `postpack` and leaving the repo manifest rewritten for - * @semantic-release/git to commit — the exact failure the backup below exists - * to prevent. - * - * The per-specifier decision is exported and takes its version lookup as an - * argument, and `main` only runs when this file is executed directly. That - * seam exists for one reason (#435): this script and - * scripts/check-conformance.mjs encode the same rule about which pnpm shapes - * are resolvable, and a shape this script REFUSES but the check EXCUSES is a - * green CI with a red release. That inversion happened twice during review of - * #417 — once for `catalog:`, once for the alias form — so a test now drives - * both real implementations and asserts they agree shape for shape. It can - * only do that if the decision is callable without packing anything. - */ -import fs from 'node:fs'; -import path from 'node:path'; -import process from 'node:process'; -import { fileURLToPath, pathToFileURL } from 'node:url'; - -const defaultPkgRoot = path.dirname( - path.dirname(fileURLToPath(import.meta.url)) -); - -const DEP_SECTIONS = [ - 'dependencies', - 'devDependencies', - 'peerDependencies', - 'optionalDependencies', -]; - -/** - * Raised for a specifier this script will not resolve. Thrown rather than - * exited so the decision can be exercised by a test; `main` turns it back into - * the same message-and-exit-1 the CLI has always produced. - */ -export class UnsupportedSpecifierError extends Error { - constructor(message) { - super(message); - this.name = 'UnsupportedSpecifierError'; - } -} - -/** - * Version of a workspace package, read through the linked node_modules entry. - * Curried over the package root so a test can supply its own lookup instead of - * needing a real pnpm-linked tree. - */ -export function makeWorkspaceVersionResolver(pkgRoot = defaultPkgRoot) { - return name => { - const linked = path.join(pkgRoot, 'node_modules', name, 'package.json'); - if (!fs.existsSync(linked)) { - throw new UnsupportedSpecifierError( - `pack-manifest: cannot resolve the workspace dependency "${name}" — ` + - `${path.relative(pkgRoot, linked)} does not exist.\n` + - 'Run `pnpm install` from the repo root before packing.' - ); - } - const { version } = JSON.parse(fs.readFileSync(linked, 'utf8')); - if (!version) { - throw new UnsupportedSpecifierError( - `pack-manifest: "${name}" has no version in its manifest` - ); - } - return version; - }; -} - -/** - * `workspace:^` / `workspace:~` / `workspace:*` take the prefix from the - * protocol and the version from the linked package; `workspace:` - * (e.g. `workspace:^5.0.0`) already carries its own range, so just unwrap it. - * A bare `workspace:` is pnpm's shorthand for `workspace:*` — leaving it to the - * unwrap branch would emit an empty specifier. - */ -export function resolveSpecifier(name, spec, resolveVersion, label = name) { - const rest = spec.slice('workspace:'.length); - if (rest === '*' || rest === '') return resolveVersion(name); - if (rest === '^' || rest === '~') return `${rest}${resolveVersion(name)}`; - // `workspace:@` is pnpm's alias form, and it does NOT publish as - // a bare range: pnpm emits `npm:@`. Unwrapping it would write - // "@scope/pkg@^5", which no package manager can install — #412 again, wearing - // a different hat. A semver range never contains "/" or a non-leading "@", - // so this only catches the alias form. - if (rest.includes('/') || rest.lastIndexOf('@') > 0) { - throw new UnsupportedSpecifierError( - `pack-manifest: ${label} is "${spec}", pnpm's alias form, which ` + - `publishes as \`npm:@\`.\n` + - 'This script does not synthesize that. Depend on the package under its ' + - 'real name, or teach pack-manifest.mjs the npm: alias rewrite.' - ); - } - return rest; -} - -/** - * The whole per-specifier decision, in one place: what this script does with - * any one dependency entry. - * - * Returns the rewritten specifier, or `null` when the entry is none of this - * script's business and should be left exactly as written. Throws - * UnsupportedSpecifierError for the shapes it refuses. - * - * This is the function scripts/pack-manifest.test.mjs drives, and it is the - * single source of "refused or resolved" that check-conformance.mjs's - * UNRESOLVABLE_AT_PACK has to stay in step with (#435). - */ -export function rewriteSpecifier(name, spec, resolveVersion, label = name) { - if (typeof spec !== 'string') return null; - // `catalog:` is the other protocol `pnpm publish` resolves and `npm publish` - // ships verbatim — the exact shape of #412. This script cannot resolve it - // (the range lives in pnpm-workspace.yaml, not in the linked package), so - // fail the release rather than pack a broken tarball. - if (spec.startsWith('catalog:')) { - throw new UnsupportedSpecifierError( - `pack-manifest: ${label} is "${spec}", and this script cannot resolve ` + - `the catalog: protocol.\n` + - 'Give it a plain semver range, or teach pack-manifest.mjs to read ' + - '`catalog`/`catalogs` from pnpm-workspace.yaml.' - ); - } - if (!spec.startsWith('workspace:')) return null; - return resolveSpecifier(name, spec, resolveVersion, label); -} - -/** - * The CLI. Wrapped in a function and guarded below so importing this module - * for the agreement test does not pack anything, and so the refusals above can - * throw (testable) while the command still exits 1 with the same message. - */ -export function main( - argv = process.argv.slice(2), - { pkgRoot = defaultPkgRoot } = {} -) { - const manifest = path.join(pkgRoot, 'package.json'); - const backup = path.join(pkgRoot, 'package.json.pack-backup'); - const resolveVersion = makeWorkspaceVersionResolver(pkgRoot); - const mode = argv[0]; - - if (mode === 'prepack') { - if (fs.existsSync(backup)) { - console.error( - 'pack-manifest: package.json.pack-backup already exists — a previous ' + - 'pack did not finish.\n' + - 'Restore the workspace manifest first: mv package.json.pack-backup package.json' - ); - return 1; - } - - const pkg = JSON.parse(fs.readFileSync(manifest, 'utf8')); - const rewritten = []; - - for (const section of DEP_SECTIONS) { - for (const [name, spec] of Object.entries(pkg[section] ?? {})) { - let resolved; - try { - resolved = rewriteSpecifier( - name, - spec, - resolveVersion, - `${section}.${name}` - ); - } catch (err) { - if (!(err instanceof UnsupportedSpecifierError)) throw err; - console.error(err.message); - return 1; - } - if (resolved === null) continue; - pkg[section][name] = resolved; - rewritten.push(`${section}.${name}: ${spec} -> ${resolved}`); - } - } - - if (!rewritten.length) { - console.log('pack-manifest: no workspace: specifiers to resolve'); - return 0; - } - - fs.copyFileSync(manifest, backup); - // Keep npm's own formatting (2-space + trailing newline) so the restored - // file and the packed one differ only in the specifiers. - fs.writeFileSync(manifest, `${JSON.stringify(pkg, null, 2)}\n`); - console.log( - `pack-manifest: resolved ${rewritten.length} workspace specifier(s)` - ); - for (const line of rewritten) console.log(` ${line}`); - return 0; - } - - if (mode === 'postpack') { - if (!fs.existsSync(backup)) { - // prepack exits early when there is nothing to rewrite; not an error. - console.log('pack-manifest: no backup to restore'); - return 0; - } - fs.copyFileSync(backup, manifest); - fs.rmSync(backup); - console.log('pack-manifest: workspace manifest restored'); - return 0; - } - - console.error('Usage: node scripts/pack-manifest.mjs '); - return 1; -} - -if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { - process.exitCode = main(); -} diff --git a/docs/docs/guides/getting-started/contributing.md b/docs/docs/guides/getting-started/contributing.md index bcb9ffbc6..7db22d250 100644 --- a/docs/docs/guides/getting-started/contributing.md +++ b/docs/docs/guides/getting-started/contributing.md @@ -142,8 +142,12 @@ no `npm publish`, no tag, no GitHub release. :::tip Safe to run; never publishes Everything above is safe. The only things that actually publish are `pnpm exec semantic-release` -**without** `--dry-run` (CI-only, on merge to `main`) and a manual `npm publish` — neither of which -is in this list. +**without** `--dry-run` (CI-only, on merge to `main`) and a manual `npm publish` / `pnpm publish` — +none of which is in this list. `bestax-migrate` publishes with `pnpm publish`, and its +`prepack` and `prepublishOnly` hooks refuse the publishers they recognise as not being pnpm, so a +stray `npm publish` or `npm pack` there exits with an explanation rather than producing an +unresolved `workspace:` specifier (#412). Both hooks are skipped by `--ignore-scripts`, and neither travels with a +tarball that was packed elsewhere. ::: ## Workflow & conventions diff --git a/package.json b/package.json index 01354370a..8f8a0dcdd 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "gen:mcp": "node scripts/gen-mcp-index.mjs", "gen:mcp:check": "node scripts/gen-mcp-index.mjs && git add --intent-to-add --all bestax-mcp/data && git diff --exit-code -- bestax-mcp/data", "gen": "pnpm gen:api-docs && pnpm gen:catalog && pnpm gen:mcp", - "all": "turbo run build typecheck test test:coverage bundle:stats && pnpm run lint && pnpm run format:check && turbo run build-storybook --filter=@allxsmith/bestax-bulma", + "all": "turbo run build typecheck test test:coverage bundle:stats && node --test \"scripts/*.test.mjs\" && pnpm run lint && pnpm run format:check && turbo run build-storybook --filter=@allxsmith/bestax-bulma", "storybook": "turbo run storybook --filter=@allxsmith/bestax-bulma", "docs": "turbo run docs --filter=@allxsmith/bestax-docs", "release": "turbo run release", @@ -36,6 +36,7 @@ "@eslint-react/eslint-plugin": "^5.18.0", "@eslint/js": "^10.0.1", "@semantic-release/changelog": "^7.0.0", + "@semantic-release/exec": "^7.1.0", "@semantic-release/git": "^11.0.1", "@semantic-release/github": "^12.0.9", "@semantic-release/npm": "^13.1.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 10476459b..5311363c4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -43,6 +43,9 @@ importers: '@semantic-release/changelog': specifier: ^7.0.0 version: 7.0.0(semantic-release@25.0.8(typescript@6.0.3)) + '@semantic-release/exec': + specifier: ^7.1.0 + version: 7.1.0(semantic-release@25.0.8(typescript@6.0.3)) '@semantic-release/git': specifier: ^11.0.1 version: 11.0.1(semantic-release@25.0.8(typescript@6.0.3)) @@ -3473,6 +3476,12 @@ packages: resolution: {integrity: sha512-mgdxrHTLOjOddRVYIYDo0fR3/v61GNN1YGkfbrjuIKg/uMgCd+Qzo3UAXJ+woLQQpos4pl5Esuw5A7AoNlzjUQ==} engines: {node: '>=18'} + '@semantic-release/exec@7.1.0': + resolution: {integrity: sha512-4ycZ2atgEUutspPZ2hxO6z8JoQt4+y/kkHvfZ1cZxgl9WKJId1xPj+UadwInj+gMn2Gsv+fLnbrZ4s+6tK2TFQ==} + engines: {node: '>=20.8.1'} + peerDependencies: + semantic-release: '>=24.1.0' + '@semantic-release/git@11.0.1': resolution: {integrity: sha512-Zr8BUYCTZMc8V6wDKN2dpR7nJgewd9I6THL3ydLTnp3OEdTo1/4RBLNYaeRucYMsjMv+BXoCNfXA0NADj1kwhw==} engines: {node: ^22.22.2 || >=24.15} @@ -14479,6 +14488,18 @@ snapshots: '@semantic-release/error@4.0.0': {} + '@semantic-release/exec@7.1.0(semantic-release@25.0.8(typescript@6.0.3))': + dependencies: + '@semantic-release/error': 4.0.0 + aggregate-error: 3.1.0 + debug: 4.4.3 + execa: 9.6.1 + lodash-es: 4.18.1 + parse-json: 8.3.0 + semantic-release: 25.0.8(typescript@6.0.3) + transitivePeerDependencies: + - supports-color + '@semantic-release/git@11.0.1(semantic-release@25.0.8(typescript@6.0.3))': dependencies: '@semantic-release/error': 4.0.0 diff --git a/scripts/check-conformance.mjs b/scripts/check-conformance.mjs index f12cc2bd8..d9e3a9620 100644 --- a/scripts/check-conformance.mjs +++ b/scripts/check-conformance.mjs @@ -33,15 +33,16 @@ * the same thing in all three deliberate copies * (CLAUDE_MD template + both JSX-generating skills), * and names only props that really exist - * publishable-manifests no published package ships a `workspace:`/`catalog:` - * specifier consumers would have to resolve (#412) + * publishable-manifests no published package ships a specifier consumers + * cannot resolve (#412). Which packages publish with + * `pnpm publish` is declared, not inferred (#436) * bypass-expiry every supply-chain bypass in pnpm-workspace.yaml * carries a `# bestax:review ` or * `# bestax:permanent` marker, and no review date has * passed (#391) */ import { readFile, readdir, writeFile, access } from 'node:fs/promises'; -import { join, relative, dirname } from 'node:path'; +import { join, relative, dirname, isAbsolute } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; // `registerVarsKeys` lives in lib/ so the API-docs generator can share the same @@ -49,6 +50,9 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; // variable tables need. Key extraction is byte-for-byte the behaviour this file // used to implement inline (verified against all 26 partials). import { registerVarsKeys } from './lib/scss-vars.mjs'; +// The inverse of the quoting bestax-migrate/release.config.js uses to build its +// exec commands. Shared so the two halves cannot drift (#436). +import { tokenize } from './lib/shell-words.mjs'; import { readRegions, sectionSpans } from './lib/api-page.mjs'; import { renderPage } from './gen-api-docs.mjs'; import { @@ -983,7 +987,7 @@ async function checkInlineStyle(updateBaseline) { // The `packages:` list out of pnpm-workspace.yaml. Reads only that block — // other keys in the file (minimumReleaseAgeExclude, publicHoistPattern) are // lists too, so scanning the whole file for `- item` would pick them up. -function parseWorkspacePackages(yaml) { +export function parseWorkspacePackages(yaml) { const dirs = []; let inBlock = false; for (const line of yaml.split(/\r?\n/)) { @@ -1001,70 +1005,307 @@ function parseWorkspacePackages(yaml) { /** * `npm publish` — which is what @semantic-release/npm shells out to — does NOT - * resolve pnpm's `workspace:` protocol the way `pnpm publish` does. A - * `workspace:^` left in a published manifest is uninstallable by every package - * manager (`EUNSUPPORTEDPROTOCOL`); that shipped as bestax-migrate@1.0.0 - * (#412), invisibly, because nothing in CI installs the published artifact. + * resolve pnpm's `workspace:` protocol. A `workspace:^` left in a published + * manifest is uninstallable by every package manager (`EUNSUPPORTEDPROTOCOL`); + * that shipped as bestax-migrate@1.0.0 (#412), invisibly, because nothing in + * CI installs the published artifact. * - * `catalog:` has the same asymmetry — pnpm resolves it at pack time, npm does - * not — so it is guarded here too, before the repo grows its first catalog. + * bestax-migrate publishes with `pnpm publish` instead (#436), which resolves + * those protocols at pack time, so it is exempt from part of this rule. Which + * packages those are is DECLARED below, not inferred from their release config. * - * Two rules, both about the manifest as CONSUMERS see it: - * 1. Sections npm resolves for consumers must have no pack-time protocol at - * all. A workspace package needed at runtime has to be a plain semver - * range. - * 2. One left in devDependencies is safe to install but still wrong to - * publish, so the package must resolve it at pack time. That escape hatch - * is `workspace:`-only: pack-manifest.mjs fails on `catalog:` rather than - * resolving it, so wiring up the hooks does not redeem a catalog spec. + * That is the whole design, and it is worth saying why, because the obvious + * alternative was tried and failed four times. Reading `release.config.js` and + * working out how a package publishes means modelling semantic-release's config + * format: a plugin may be a bare string, a `[name, config]` tuple, or a + * `{ path, ...config }` object; a step may be an array or a single one of those; + * the config may live in eight different filenames or in package.json; the + * command may be `publishCmd` or the generic `cmd`, and may say `pnpm publish` + * or `pnpm --filter x publish`. Every one of those was missed at some point by + * a parser that looked obviously correct, and every miss fell through to the + * exempt branch — the one verdict that switches the rule OFF. A parser that + * fails open is worse than no parser. + * + * So the exemption is a declaration, which cannot be misparsed, and + * scripts/publishable-manifests.test.mjs checks the declaration against what + * the release configs actually do. Getting THAT wrong fails a test, loudly, + * instead of silently exempting a package. */ -const PACK_TIME_PROTOCOLS = ['workspace:', 'catalog:']; +const PNPM_PUBLISHED = new Set(['bestax-migrate']); -const hasPackTimeProtocol = spec => - typeof spec === 'string' && PACK_TIME_PROTOCOLS.some(p => spec.startsWith(p)); +/** + * Protocols that mean something inside this workspace and are not a plain + * installable specifier once published. + * + * `link:`, `portal:` and `file:` are here because NEITHER publisher rewrites + * them: pnpm's export converter chain is workspace/catalog/jsr (verified in the + * 11.9.0 bundle), and npm has no notion of the first two at all. A published + * manifest carrying one points at a path that does not exist on any consumer's + * machine. + */ +const PACK_TIME_PROTOCOLS = [ + 'workspace:', + 'catalog:', + 'jsr:', + 'link:', + 'portal:', + 'file:', +]; -// The two shapes the pack hooks refuse rather than guess at, so devDependencies -// carrying them are violations no matter how the hooks are wired. Kept in step -// with bestax-migrate/scripts/pack-manifest.mjs, which refuses both. -// -// "Kept in step" used to mean "by reading both files carefully", and that -// failed twice during review of #417 — once for `catalog:`, once for the alias -// form. A shape the script REFUSES but this check EXCUSES is a green CI with a -// red release, which is the exact inversion this check exists to prevent. So -// these are exported and scripts/pack-manifest.test.mjs now drives them against -// the real resolver, asserting the two agree shape for shape (#435). -export const UNRESOLVABLE_AT_PACK = [ - { - matches: spec => spec.startsWith('catalog:'), - why: - 'pack-manifest.mjs cannot resolve catalog: — the range lives in ' + - 'pnpm-workspace.yaml, not in the linked package', - }, - { - // `workspace:@`. A semver range holds neither "/" nor a - // non-leading "@", so this does not catch `workspace:^5.0.0`. - matches: spec => { - if (!spec.startsWith('workspace:')) return false; - const rest = spec.slice('workspace:'.length); - return rest.includes('/') || rest.lastIndexOf('@') > 0; - }, - why: - "pnpm's alias form publishes as `npm:@`, which " + - 'pack-manifest.mjs does not synthesize', - }, +/** + * The subset `pnpm publish` turns into a plain, installable semver range. That + * is what makes a pnpm publisher safe to exempt, and it is not all of them: + * pnpm rewrites `jsr:@scope/pkg@^1` to `npm:@jsr/scope__pkg@^1`, which resolves + * only for a consumer who has configured the @jsr registry. + */ +const PNPM_RESOLVES_TO_PLAIN_RANGE = ['workspace:', 'catalog:']; + +const DEP_SECTIONS = [ + 'dependencies', + 'devDependencies', + 'peerDependencies', + 'optionalDependencies', +]; + +/** Sections a consumer of the published package resolves. */ +const CONSUMER_SECTIONS = DEP_SECTIONS.filter(s => s !== 'devDependencies'); + +const packTimeProtocol = spec => + typeof spec === 'string' + ? PACK_TIME_PROTOCOLS.find(p => spec.startsWith(p)) + : undefined; + +/** + * The per-package rule, split out from the filesystem walk so it can be driven + * with fixtures. Without this seam the violation branches never execute during + * a real run — bestax-migrate is the only package carrying a pack-time + * specifier and it is exempt for it — so inverting the rule would leave CI + * green. + * + * Each offender carries the reason it survived the filter, so the message does + * not re-derive the predicate that produced it. Two copies of one rule inside + * one function is the drift this repo keeps paying for. + */ +export function manifestViolations(dir, pkg) { + if (pkg?.private) return []; + + // The declaration is consulted HERE rather than by the caller, so that a test + // driving this function also exercises the wiring. Passing the verdict in + // meant a walk that ignored the declaration entirely still passed every test. + const publishesWithPnpm = PNPM_PUBLISHED.has(dir); + + const violations = []; + + // The exemption assumes pnpm packs this package. In CI that is the release + // config's job; everywhere else it is the prepublishOnly guard's, and a + // package can otherwise gain the exemption and lose the guard in one edit. + // Checked here, alongside the exemption it compensates for, so a test that + // covers one covers the other. + // Both hooks, and the command has to RUN the guard rather than mention it: + // `echo skipping require-pnpm-publish.mjs` satisfied a substring test while + // the exemption stayed granted. `prepack` matters as much as + // `prepublishOnly`, because `npm pack` runs only the former and its tarball + // can then be published directly. + const runsGuard = hook => { + const cmd = pkg?.scripts?.[hook]; + if (typeof cmd !== 'string') return false; + let words; + try { + words = tokenize(cmd); + } catch { + return false; + } + // The guard must run before anything else can short-circuit past it, but + // "first" is about ORDER, not about the exact spelling: `node --flag x` + // and `pnpm node x` both execute it, and demanding a literal `node ` + // reported those as missing with no satisfying form to offer. + const i = words.findIndex(w => w.endsWith('require-pnpm-publish.mjs')); + if (i < 1) return false; + // Everything ahead of it must be an interpreter or a flag. A separator + // there means something else ran, or could run instead of, the guard. + return words + .slice(0, i) + .every(w => w.startsWith('-') || /^(node|pnpm|exec)$/.test(w)); + }; + + const missingGuard = publishesWithPnpm + ? ['prepack', 'prepublishOnly'].filter(h => !runsGuard(h)) + : []; + + if (missingGuard.length) { + violations.push( + `${dir} is declared in PNPM_PUBLISHED, but does not run ` + + `scripts/require-pnpm-publish.mjs on ${missingGuard.join(' or ')}. ` + + `The exemption ` + + `then holds only for releases from CI: a hand-run \`npm publish\` in ` + + `that directory would ship the specifier verbatim (#412). Add ` + + `${missingGuard.map(h => `"${h}"`).join(' and ')}: "node ${relative( + join(REPO, dir), + join(REPO, 'scripts', 'require-pnpm-publish.mjs') + )}".` + ); + } + + const offenders = []; + for (const section of DEP_SECTIONS) { + for (const [name, spec] of Object.entries(pkg?.[section] ?? {})) { + const protocol = packTimeProtocol(spec); + if (!protocol) continue; + + if (!publishesWithPnpm) { + offenders.push({ section, name, spec, protocol, why: 'npm' }); + continue; + } + if (!PNPM_RESOLVES_TO_PLAIN_RANGE.includes(protocol)) { + offenders.push({ section, name, spec, protocol, why: 'unresolved' }); + continue; + } + // pnpm turns this into a real range, so it installs. It is still wrong in + // a section consumers resolve: it makes everyone installing this package + // install that one too. + if (CONSUMER_SECTIONS.includes(section)) { + offenders.push({ section, name, spec, protocol, why: 'consumer' }); + } + } + } + + violations.push( + ...offenders.map(({ section, name, spec, protocol, why }) => { + const head = `${dir}/package.json declares "${name}": "${spec}" in ${section}. `; + if (why === 'npm') { + // Both halves have to match the protocol. `file:` is not an + // EUNSUPPORTEDPROTOCOL — npm understands it perfectly and resolves it + // to a path that exists on this machine and no consumer's. And + // suggesting a move to `pnpm publish` is only useful for the protocols + // pnpm turns into a range; for the rest it sends the maintainer through + // a publish migration that lands on the same specifier. + const pnpmWouldFixIt = PNPM_RESOLVES_TO_PLAIN_RANGE.includes(protocol); + return ( + head + + `${dir} publishes with \`npm publish\`, which does not resolve ` + + `${protocol} ` + + (protocol === 'file:' + ? 'into anything a consumer can use: it points at a path that ' + + 'exists here and on none of their machines' + : 'at all, so the published package would be uninstallable ' + + '(EUNSUPPORTEDPROTOCOL, #412)') + + '. Give it a plain semver range' + + (pnpmWouldFixIt + ? `, or move ${dir} to \`pnpm publish\` and declare it in ` + + `PNPM_PUBLISHED (#436).` + : '. `pnpm publish` does not resolve it either.') + ); + } + if (why === 'unresolved') { + if (section === 'devDependencies') { + // No consumer resolves a dependency's devDependencies, so the + // consumer-facing complaint does not apply and "give it a semver + // range" is not even possible for a local path. It is still worth a + // human decision, because it means nothing outside this workspace. + return ( + head + + `\`pnpm publish\` does not rewrite ${protocol}, so the published ` + + `manifest carries a specifier that means nothing outside this ` + + `workspace. Nothing installs it (consumers do not resolve ` + + `devDependencies), but it should not ship: drop it, or depend on ` + + `the package by name.` + ); + } + const detail = + protocol === 'jsr:' + ? 'rewrites it to an aliased `npm:@jsr/…` specifier, which resolves ' + + 'only for consumers who have configured the @jsr registry' + : 'does not rewrite it at all, so no consumer can resolve it'; + return ( + head + `\`pnpm publish\` ${detail}. Give it a plain semver range.` + ); + } + return ( + head + + `\`pnpm publish\` resolves ${protocol} to a real range, so it installs, ` + + `but ${section} is resolved by consumers and this specifier only means ` + + `something inside the workspace. ` + + (section === 'peerDependencies' + ? // A peer dep is SUPPOSED to reach consumers, so "move it to + // devDependencies" would break the contract rather than fix it. + 'A peer dependency is meant to reach them, so give it an explicit ' + + 'semver range naming the versions this package actually supports.' + : `Move it to devDependencies if only this package's own build or ` + + `tests need it, or give it a plain semver range if consumers ` + + `really do need it at runtime.`) + ); + }) + ); + + return violations; +} + +/** + * Lifecycle hooks that run during a pack or publish, and therefore name scripts + * whose absence would fail a release rather than CI. + * + * Deliberately not every script: `"start": "node dist/index.js"` is a correct + * entry naming a build OUTPUT, and this check runs before the build in ci.yml, + * so demanding it exist would red the pipeline on a working config. + * + * `prepare` is excluded for the same reason and it is not obvious: it runs + * after the build during a pack, so `"prepare": "node ./dist/postbuild.mjs"` is + * a normal entry whose target legitimately does not exist when this runs. + */ +const LIFECYCLE_HOOKS = [ + 'prepublishOnly', + 'prepublish', + 'prepack', + 'postpack', + 'publish', + 'postpublish', ]; -export const unresolvableAtPack = spec => - typeof spec === 'string' && - UNRESOLVABLE_AT_PACK.find(rule => rule.matches(spec)); +/** + * Script paths a lifecycle hook names. Path-shaped words only: a bare + * `bundle.js` or a `--require=./polyfill.js` flag is not a script to demand + * exists. + * + * Extensions rather than "anything path-shaped", because a hook may legitimately + * name a directory or a non-file argument. `.ts` and `.sh` are included since + * `tsx ./x.ts` and `bash ./x.sh` name a script exactly as much as `node ./x.mjs`. + */ +const SCRIPT_EXT = /\.(mjs|cjs|js|ts|mts|cts|sh)$/; + +export function hookScripts(pkg) { + const referenced = new Set(); + for (const hook of LIFECYCLE_HOOKS) { + const cmd = pkg?.scripts?.[hook]; + if (typeof cmd !== 'string') continue; + let words; + try { + words = tokenize(cmd); + } catch { + // An unbalanced quote is a command the shell would reject anyway. + // Reporting a path invented from it would be a violation about a file + // nobody named. + continue; + } + for (const word of words) { + if (word.startsWith('-')) continue; + // A build OUTPUT is not a script to demand exists: this check runs + // before the build in ci.yml, and `"prepack": "node ./dist/stamp.mjs"` + // is a working config. `prepare` was excluded wholesale for this, but + // prepack/postpack/publish run at the same moment and needed the same + // allowance. + if (/(^|\/)(dist|build|lib|out|es|esm|cjs)\//.test(word)) continue; + // A path built from a shell variable cannot be resolved here, and + // access()ing the literal text would report a working hook as broken. + if (word.includes('$')) continue; + if (!word.includes('/')) continue; + if (SCRIPT_EXT.test(word)) referenced.add(word); + } + } + return [...referenced]; +} async function checkPublishableManifests() { const violations = []; - const CONSUMER_SECTIONS = [ - 'dependencies', - 'peerDependencies', - 'optionalDependencies', - ]; const packages = parseWorkspacePackages( await readFile(join(REPO, 'pnpm-workspace.yaml'), 'utf8') @@ -1074,10 +1315,9 @@ async function checkPublishableManifests() { } for (const dir of packages) { - const manifestPath = join(REPO, dir, 'package.json'); let pkg; try { - pkg = JSON.parse(await readFile(manifestPath, 'utf8')); + pkg = JSON.parse(await readFile(join(REPO, dir, 'package.json'), 'utf8')); } catch { violations.push( `pnpm-workspace.yaml lists "${dir}" but ${dir}/package.json is missing ` + @@ -1085,86 +1325,24 @@ async function checkPublishableManifests() { ); continue; } - if (pkg.private) continue; - - for (const section of CONSUMER_SECTIONS) { - for (const [name, spec] of Object.entries(pkg[section] ?? {})) { - if (hasPackTimeProtocol(spec)) { - const protocol = spec.slice(0, spec.indexOf(':') + 1); - violations.push( - `${dir}/package.json declares "${name}": "${spec}" in ${section}. ` + - `npm publish does not resolve the ${protocol} protocol, so the ` + - `published package is uninstallable (EUNSUPPORTEDPROTOCOL, #412). ` + - `Move it to devDependencies if it is only needed to build or ` + - `test this package, or give it a plain semver range if consumers ` + - `really need it at runtime.` - ); - } - } - } - - const devDeps = pkg.devDependencies ?? {}; - - // Hooks present is no defence for the shapes pack-manifest.mjs refuses: - // the check would go green and the release would be what breaks. So these - // are violations on their own terms, reported alongside the hook rules - // below rather than instead of them. - for (const [name, spec] of Object.entries(devDeps)) { - const rule = unresolvableAtPack(spec); - if (!rule) continue; - violations.push( - `${dir}/package.json declares "${name}": "${spec}" in ` + - `devDependencies. The prepack/postpack hooks do not make that ` + - `publishable — ${rule.why} — so the pack fails instead (#412). ` + - `Give it a plain semver range.` - ); - } - - // A plain `workspace:` range is the one case the hooks genuinely cover, so - // it is the only one whose violation they suppress. - const stillUnresolved = Object.values(devDeps).some( - spec => - typeof spec === 'string' && - spec.startsWith('workspace:') && - !unresolvableAtPack(spec) - ); - if (!stillUnresolved) continue; - - // Deliberately matched by name: only pack-manifest.mjs is known to perform - // this rewrite. A package with its own differently-named pack hook (e.g. - // bulma-ui/scripts/pack-pointer-files.mjs) should call pack-manifest.mjs as - // well rather than be waved through — a false positive here costs one line - // of config, a false negative ships another uninstallable tarball. - const hookScripts = ['prepack', 'postpack'].map(hook => - (pkg.scripts?.[hook] ?? '') - .split(/\s+/) - .find(token => token.endsWith('pack-manifest.mjs')) - ); - - if (!hookScripts.every(Boolean)) { - violations.push( - `${dir}/package.json keeps a workspace: specifier in devDependencies ` + - `but does not resolve it at pack time, so it would be published ` + - `verbatim (#412). Add "prepack": "node scripts/pack-manifest.mjs ` + - `prepack" and the matching postpack hook — copy ` + - `bestax-migrate/scripts/pack-manifest.mjs.` - ); - continue; - } - - // Naming the script is not the same as shipping it. A hook left pointing at - // a moved or deleted path satisfies the check above and then fails at - // `npm publish` — the one moment where a failure is most expensive. - for (const rel of new Set(hookScripts)) { + // The private check lives in manifestViolations, not here, so there is one + // copy of it. hookScripts still runs for private packages: a broken pack + // hook is worth reporting whether or not the package publishes. + violations.push(...manifestViolations(dir, pkg)); + + // Naming a script is not the same as shipping it. A hook pointing at a + // moved path fails during the release rather than in CI, which is the one + // moment where a failure is most expensive. npm and pnpm both run lifecycle + // scripts from the package root, so these resolve from `dir`. + for (const rel of hookScripts(pkg)) { + const abs = isAbsolute(rel) ? rel : join(REPO, dir, rel); try { - await access(join(REPO, dir, rel)); + await access(abs); } catch { violations.push( - `${dir}/package.json points its prepack/postpack hooks at "${rel}", ` + - `but ${dir}/${rel} does not exist. The workspace: specifier in ` + - `devDependencies would go out unresolved (#412), and ` + - `the failure would surface during the release rather than in CI. ` + - `Restore the script or fix the path in both hooks.` + `${dir}/package.json runs "${rel}" from a lifecycle hook, but ` + + `${relative(REPO, abs)} does not exist. That would fail during ` + + `the release rather than in CI (#436).` ); } } @@ -1314,9 +1492,10 @@ async function main() { } } -// Only run the suite when invoked as a command. The rules above are imported -// by scripts/pack-manifest.test.mjs, and importing a module should not run a -// repo-wide conformance sweep as a side effect. +// Only run the suite when invoked as a command. `manifestViolations` and +// `hookScripts` above are imported by scripts/publishable-manifests.test.mjs, +// and importing a module should not run a repo-wide conformance sweep as a +// side effect. if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { main().catch(err => { console.error(err); diff --git a/scripts/lib/api-page.mjs b/scripts/lib/api-page.mjs index 11597d07c..43f8554aa 100644 --- a/scripts/lib/api-page.mjs +++ b/scripts/lib/api-page.mjs @@ -26,8 +26,6 @@ const OPEN_RE = /^\s*$/; const CLOSE_RE = /^\s*$/; -export const REGION_IDS = ['overview', 'import', 'props', 'cssvars']; - export function openMarker(id) { return ``; } @@ -170,10 +168,6 @@ export function replaceRegion(src, id, body, label = '') { return joinLines(next, crlf); } -export function hasRegion(src, id, label = '') { - return readRegions(src, label).has(id); -} - /** * Top-level (`## `) section spans, fence-aware. * diff --git a/scripts/lib/release-config.mjs b/scripts/lib/release-config.mjs new file mode 100644 index 000000000..f9c677b3c --- /dev/null +++ b/scripts/lib/release-config.mjs @@ -0,0 +1,51 @@ +/** + * Reading a package's semantic-release exec options (#436). + * + * Used only by the tests, and deliberately NOT by + * scripts/check-conformance.mjs: the check decides which packages publish with + * pnpm from an explicit declaration, precisely so that no verdict depends on + * parsing this format. Here the shape is asserted rather than inferred, so a + * config that does not match throws, and a throw is a failed test. + * + * One copy, because there were four: three in publishable-manifests.test.mjs + * and one in npm-release-info.test.mjs, each re-deriving the same lookup and + * each dereferencing `[1]` without checking it found anything. + */ +const EXEC = '@semantic-release/exec'; + +/** + * Understands the three plugin shapes semantic-release accepts, because two of + * them are shapes check-conformance.mjs records as historical misses. Returns + * null when the plugin genuinely is not there, and THROWS when it is there in + * a form this cannot read — the caller is asserting things about that config, + * so degrading to `{}` would turn a real assertion into a vacuous pass. + */ +export async function execOptions(dir) { + const { default: config } = await import(`../../${dir}/release.config.js`); + const plugins = config.plugins ?? []; + + for (const p of plugins) { + if (typeof p === 'string') { + if (p === EXEC) { + throw new Error( + `${dir}/release.config.js declares ${EXEC} with no options; there is ` + + 'nothing to read.' + ); + } + continue; + } + if (Array.isArray(p) && p[0] === EXEC) return p[1] ?? {}; + if (p && typeof p === 'object' && p.path === EXEC) { + // eslint-disable-next-line no-unused-vars + const { path: _path, ...options } = p; + return options; + } + } + return null; +} + +/** The `branches` a package releases from. */ +export async function releaseBranches(dir) { + const { default: config } = await import(`../../${dir}/release.config.js`); + return config.branches; +} diff --git a/scripts/lib/shell-words.mjs b/scripts/lib/shell-words.mjs new file mode 100644 index 000000000..1c0ad5033 --- /dev/null +++ b/scripts/lib/shell-words.mjs @@ -0,0 +1,105 @@ +/** + * POSIX shell quoting and its inverse, in one place (#436). + * + * These two are halves of one contract. `quote` builds the commands in + * bestax-migrate/release.config.js; `tokenize` is how + * scripts/check-conformance.mjs reads those same commands back to find the + * script paths they name. They lived in different files, implemented + * differently, with nothing linking them, and they disagreed: quote emits the + * `'\''` escape for an embedded apostrophe, and tokenize stripped quote + * characters blindly, so a checkout path containing an apostrophe quoted + * correctly for the shell and then tokenized into a path that does not exist. + * + * That is the same two-implementations-of-one-rule shape #435 was created to + * catch, so they are colocated and the round trip is asserted directly + * (scripts/shell-words.test.mjs) rather than left to agree by inspection. + */ + +/** Single-quote a value for /bin/sh, escaping embedded apostrophes. */ +export function quote(value) { + return `'${String(value).replace(/'/g, `'\\''`)}'`; +} + +/** + * Split a command into shell words, undoing one level of quoting. + * + * A word is a run of quoted and unquoted chunks with no whitespace between + * them, so `--dir='/My Projects/x'` stays one word rather than splitting at the + * `=`. Handles the `'\''` escape `quote` emits, and backslash escapes both + * bare and inside double quotes, which is where /bin/sh honours them. + * + * Shell operators (`;`, `|`, `&`, `>`, `<`) terminate a word even without + * whitespace, because `node ./a.mjs;node ./b.mjs` names two scripts and callers + * that scan for paths must see both. + * + * Throws on an unbalanced quote rather than inventing a word. The shell would + * reject that command outright, so silently accepting it would let a caller + * assert things about a command that cannot run. + */ +export function tokenize(cmd) { + // Subshell parens and backticks end a word too: `(cd x && node ./a.mjs)` + // otherwise yields `./a.mjs)`, which no extension test matches, so a caller + // scanning for script paths silently drops it. + const OPERATORS = new Set([';', '|', '&', '>', '<', '(', ')', '`']); + const words = []; + let word = null; + let quoteChar = null; + + const push = () => { + if (word !== null) words.push(word); + word = null; + }; + + for (let i = 0; i < cmd.length; i++) { + const ch = cmd[i]; + + if (quoteChar === "'") { + // Single quotes are literal in sh: no escapes inside them at all. + if (ch === "'") quoteChar = null; + else word += ch; + continue; + } + if (quoteChar === '"') { + if (ch === '\\' && i + 1 < cmd.length && /["\\$`]/.test(cmd[i + 1])) { + word += cmd[++i]; + } else if (ch === '"') { + quoteChar = null; + } else { + word += ch; + } + continue; + } + + if (ch === "'" || ch === '"') { + quoteChar = ch; + word ??= ''; + continue; + } + if (ch === '\\' && i + 1 < cmd.length) { + word ??= ''; + word += cmd[++i]; + continue; + } + if (OPERATORS.has(ch)) { + push(); + // Operators are their own words, so a caller splitting on them still can. + words.push(ch); + continue; + } + if (/\s/.test(ch)) { + push(); + continue; + } + word ??= ''; + word += ch; + } + + if (quoteChar) { + throw new Error( + `shell-words: unbalanced ${quoteChar === "'" ? 'single' : 'double'} ` + + `quote in: ${cmd}` + ); + } + push(); + return words; +} diff --git a/scripts/npm-release-info.mjs b/scripts/npm-release-info.mjs new file mode 100644 index 000000000..84f96ecaa --- /dev/null +++ b/scripts/npm-release-info.mjs @@ -0,0 +1,120 @@ +#!/usr/bin/env node +/** + * Prints the release object semantic-release records for an npm publish (#436). + * + * `@semantic-release/npm` returns one from its publish step, and + * `@semantic-release/github` turns it into the "The release is available on:" + * entry it comments onto every linked issue and PR. Handing publishing to + * `pnpm publish` loses that, and loses it in a way that looks like a bug rather + * than an omission: `@semantic-release/exec` parses its command's stdout as + * JSON, pnpm prints prose, so the parse fails and exec returns `undefined`. + * `undefined` is not `false`, so semantic-release's publish transform + * (lib/definitions/plugins.js) falls through to spreading `nextRelease` over + * it, and `nextRelease.name` is the git tag (index.js:187). The comment then + * reads: + * + * The release is available on: + * - `bestax-migrate@2.0.1` <- a bare tag, no link + * - [GitHub release](...) + * + * which reads as a link that failed to render. So print the same shape + * @semantic-release/npm does, and let the publish command send pnpm's own + * output to stderr. Nothing is hidden by that: exec pipes stdout and stderr + * separately to the job log, so both still appear, and a failed publish still + * throws. + * + * Name and URL deliberately match `@semantic-release/npm/lib/get-release-info.js` + * exactly, so bestax-migrate's comments are indistinguishable from the three + * packages still publishing through that plugin. + */ +import fs from 'node:fs'; +import path from 'node:path'; +import process from 'node:process'; +import { pathToFileURL } from 'node:url'; + +export function releaseInfo( + pkgName, + version, + distTag = 'latest', + registry = undefined +) { + if (!pkgName) throw new Error('npm-release-info: package has no name'); + if (!version) { + // Better to fail the publish step than to comment a URL pointing at a + // version that does not exist. + throw new Error( + 'npm-release-info: no version given. Pass ${nextRelease.version} from ' + + 'the publishCmd template.' + ); + } + // Upstream omits the url entirely for a non-default registry rather than + // linking to npmjs.com, and matching that matters: a link to a package page + // that does not exist is worse than no link. + // + // The limit of that, stated rather than left to be discovered: the registry + // is known here only from `publishConfig.registry`. pnpm resolves the real + // one as `publishConfig.registry ?? registries['@scope'] ?? registries.default`, + // and the last two come from npmrc, which this cannot see. A registry set + // purely in npmrc would therefore still produce an npmjs.com link. The repo's + // own .npmrc pins `registry=https://registry.npmjs.org/`, so the two agree + // today; a future non-default registry has to pass it here explicitly. + const onNpmjs = + !registry || /^https?:\/\/registry\.npmjs\.org\/?$/.test(registry); + return { + name: `npm package (@${distTag} dist-tag)`, + url: onNpmjs + ? `https://www.npmjs.com/package/${pkgName}/v/${version}` + : undefined, + channel: distTag, + }; +} + +export function main(argv = process.argv.slice(2), cwd = process.cwd()) { + // `--dir` decouples this from the cwd semantic-release happens to run in. + // Both exec commands pass an absolute path, so the release does not depend + // on being invoked from the package directory. + const dirFlag = argv.find(a => a.startsWith('--dir=')); + const root = dirFlag ? dirFlag.slice('--dir='.length) : cwd; + const { name, publishConfig } = JSON.parse( + fs.readFileSync(path.join(root, 'package.json'), 'utf8') + ); + const [version, distTag] = argv.filter(a => !a.startsWith('--')); + return JSON.stringify( + releaseInfo(name, version, distTag || 'latest', publishConfig?.registry) + ); +} + +/** + * The CLI, which deliberately CANNOT fail. + * + * publishCmd chains this after `pnpm publish` with `&&`, so a non-zero exit + * here would throw out of @semantic-release/exec's publish step after the + * tarball is already on the registry: @semantic-release/github never runs, the + * job goes red, and the version is spent. For a link in a comment. So any + * failure degrades to `{}`, which puts the comment back to the bare-tag + * rendering this script improves on rather than taking a successful release + * down with it. The reason is still printed, on stderr, where the job log + * shows it. + */ +export function cli( + argv = process.argv.slice(2), + cwd = process.cwd(), + warn = console.error +) { + try { + return { stdout: main(argv, cwd), code: 0 }; + } catch (err) { + warn( + `npm-release-info: ${err.message}\n` + + 'The package published successfully; only the npm link in the release ' + + 'comment is affected.' + ); + return { stdout: '{}', code: 0 }; + } +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + const { stdout, code } = cli(); + console.log(stdout); + process.exitCode = code; +} diff --git a/scripts/npm-release-info.test.mjs b/scripts/npm-release-info.test.mjs new file mode 100644 index 000000000..52f077ff8 --- /dev/null +++ b/scripts/npm-release-info.test.mjs @@ -0,0 +1,142 @@ +/** + * Covers scripts/npm-release-info.mjs (#436). + * + * The output shape is the contract: @semantic-release/github renders it into + * the comment posted on every linked issue and PR, and it has to be + * indistinguishable from what @semantic-release/npm produces for the three + * packages still publishing that way. So the field names and the URL format + * are pinned against that plugin's get-release-info.js. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { fileURLToPath } from 'node:url'; + +import { releaseInfo, main, cli } from './npm-release-info.mjs'; +import { execOptions } from './lib/release-config.mjs'; + +// fileURLToPath, not .pathname: a URL path is percent-encoded, so a checkout +// under a directory with a space resolves to a path that does not exist. +const MIGRATE = fileURLToPath(new URL('../bestax-migrate/', import.meta.url)); + +test('the shape matches @semantic-release/npm/lib/get-release-info.js', () => { + const info = releaseInfo('bestax-migrate', '2.0.1'); + assert.deepEqual(info, { + name: 'npm package (@latest dist-tag)', + url: 'https://www.npmjs.com/package/bestax-migrate/v/2.0.1', + channel: 'latest', + }); +}); + +test('a scoped name keeps its slash in the URL', () => { + // npmjs.com URLs carry the scope unencoded; encoding it would 404. + assert.equal( + releaseInfo('@allxsmith/bestax-bulma', '5.11.1').url, + 'https://www.npmjs.com/package/@allxsmith/bestax-bulma/v/5.11.1' + ); +}); + +test('a non-default dist-tag shows up in both the name and the channel', () => { + const info = releaseInfo('bestax-migrate', '3.0.0-next.1', 'next'); + assert.equal(info.name, 'npm package (@next dist-tag)'); + assert.equal(info.channel, 'next'); +}); + +test('a missing version fails instead of linking to a version that does not exist', () => { + // The template could silently interpolate nothing; a URL pointing at a + // non-existent version is worse than a failed step. + assert.throws(() => releaseInfo('bestax-migrate', undefined), /no version/); + assert.throws(() => releaseInfo('bestax-migrate', ''), /no version/); +}); + +test('a package with no name fails rather than emitting a broken URL', () => { + assert.throws(() => releaseInfo(undefined, '2.0.1'), /no name/); +}); + +test('main reads the real bestax-migrate manifest and emits parseable JSON', () => { + // exec runs `parseJson` over stdout, so anything unparseable here puts the + // publish back in the state this script exists to fix. + const out = main(['2.0.1'], MIGRATE); + const parsed = JSON.parse(out); + assert.equal(parsed.name, 'npm package (@latest dist-tag)'); + assert.match( + parsed.url, + /^https:\/\/www\.npmjs\.com\/package\/bestax-migrate\/v\/2\.0\.1$/ + ); + assert.equal( + out.trim(), + out, + "stdout must not need trimming beyond exec's own" + ); +}); + +test('the CLI cannot fail, however wrong its input', () => { + // publishCmd chains this after `pnpm publish` with `&&`. A non-zero exit + // would throw out of the publish step with the tarball already on the + // registry: @semantic-release/github never runs, the job reds, the version is + // spent. So every failure degrades to `{}`, which renders as the bare tag + // this script improves on rather than taking a good release down. + for (const argv of [ + [], + ['--dir=/nonexistent'], + ['2.0.1', '--dir=/nonexistent'], + ]) { + const { stdout, code } = cli(argv, MIGRATE, () => {}); + assert.equal(code, 0, `${JSON.stringify(argv)} must exit 0`); + assert.doesNotThrow(() => JSON.parse(stdout), 'stdout must stay parseable'); + } + // …and the reason still reaches the log. + let warned = ''; + cli([], MIGRATE, m => (warned = m)); + assert.match(warned, /no version/); + assert.match(warned, /published successfully/); +}); + +test('--dir decouples it from the cwd semantic-release ran in', () => { + // Both exec commands pass an absolute --dir, so the release does not depend + // on being invoked from the package directory. + const { stdout } = cli(['2.0.1', `--dir=${MIGRATE}`], '/', () => {}); + assert.equal( + JSON.parse(stdout).url, + 'https://www.npmjs.com/package/bestax-migrate/v/2.0.1' + ); +}); + +test('a non-default registry gets no npmjs.com link', () => { + // Upstream omits the url rather than linking to a page that will 404, and + // matching that is the point: a broken link presented as the release + // artifact is worse than the bare tag. + const info = releaseInfo('x', '1.0.0', 'latest', 'https://npm.example.test/'); + assert.equal(info.url, undefined); + assert.equal(info.name, 'npm package (@latest dist-tag)'); + // The default registry still links. + assert.ok( + releaseInfo('x', '1.0.0', 'latest', 'https://registry.npmjs.org/').url + ); + assert.ok(releaseInfo('x', '1.0.0').url); +}); + +test('the publishCmd sends pnpm output to stderr and calls this script', async () => { + // The redirect is what makes stdout parseable. Losing it silently reverts + // the bare-tag comment, so it is pinned rather than left to review. + const exec = await execOptions('bestax-migrate'); + assert.match(exec.publishCmd, /1>&2/); + assert.match(exec.publishCmd, /npm-release-info\.mjs/); + assert.match(exec.publishCmd, /--dir=/); + assert.match(exec.publishCmd, /\$\{nextRelease\.version\}/); + // `|| true` in the SHELL, not just error handling inside the script: node + // exits non-zero before that handling if it cannot load the file at all, and + // the `&&` chain would then fail the publish with the tarball already up. + assert.match(exec.publishCmd, /\|\| true/); + // Paths are quoted, so a checkout under a directory with a space does not + // split into two arguments. + assert.match(exec.publishCmd, /node '[^']*npm-release-info\.mjs'/); + assert.match( + exec.verifyConditionsCmd, + /node '[^']*verify-oidc-context\.mjs'/ + ); + // Absolute paths, so neither command depends on the cwd semantic-release was + // invoked from. + assert.doesNotMatch(exec.publishCmd, /\.\.\/scripts/); + assert.doesNotMatch(exec.verifyConditionsCmd, /\.\.\/scripts/); +}); diff --git a/scripts/pack-manifest.test.mjs b/scripts/pack-manifest.test.mjs deleted file mode 100644 index e88bfcddc..000000000 --- a/scripts/pack-manifest.test.mjs +++ /dev/null @@ -1,364 +0,0 @@ -/** - * Holds bestax-migrate/scripts/pack-manifest.mjs and the - * `publishable-manifests` rule in scripts/check-conformance.mjs in agreement - * (#435). - * - * Both files encode the same rule about which pnpm specifier shapes survive - * `npm publish`. The pack script decides what it will rewrite at pack time; the - * conformance check decides what it will let past CI on the strength of the - * pack hooks being wired. **A shape the script refuses but the check excuses is - * a green CI with a red release** — the check reports all clear and the release - * is what breaks, which is the exact inversion the check exists to prevent. - * - * That inversion shipped twice during review of #417, both times in code that - * read as obviously correct: `catalog:` (fixed in 4127ead) and pnpm's alias - * form `workspace:@` (fixed in de6a900, after it was found - * unwrapping to a bare `@scope/pkg@^5` that no package manager can install — - * #412 wearing a different hat). - * - * So this drives BOTH real implementations rather than restating their logic. A - * third copy of the rule in a test would be one more thing to drift, which is - * the bug class rather than a fix for it. - */ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; - -import { - main, - rewriteSpecifier, - resolveSpecifier, - UnsupportedSpecifierError, -} from '../bestax-migrate/scripts/pack-manifest.mjs'; -import { unresolvableAtPack } from './check-conformance.mjs'; - -/** The linked-package lookup, stubbed so no pnpm tree is required. */ -const VERSION = '5.11.1'; -const resolveVersion = () => VERSION; - -const NAME = '@allxsmith/bestax-bulma'; - -/** Does the pack script refuse this specifier? */ -function packRefuses(spec) { - try { - rewriteSpecifier(NAME, spec, resolveVersion); - return false; - } catch (err) { - if (err instanceof UnsupportedSpecifierError) return true; - throw err; - } -} - -/** Does the conformance check flag this specifier as unresolvable at pack time? */ -const checkFlags = spec => Boolean(unresolvableAtPack(spec)); - -/** - * Every shape pnpm documents, and what each side is supposed to do with it. - * `refused: true` means BOTH the script refuses it and the check flags it — - * that pairing is the invariant, and it is asserted as a biconditional below - * rather than as two independent expectations. - */ -const SHAPES = [ - { spec: 'workspace:*', refused: false, resolves: VERSION }, - { spec: 'workspace:', refused: false, resolves: VERSION }, - { spec: 'workspace:^', refused: false, resolves: `^${VERSION}` }, - { spec: 'workspace:~', refused: false, resolves: `~${VERSION}` }, - { spec: 'workspace:^5.0.0', refused: false, resolves: '^5.0.0' }, - { spec: 'workspace:~5.0.0', refused: false, resolves: '~5.0.0' }, - { spec: 'workspace:5.0.0', refused: false, resolves: '5.0.0' }, - { spec: 'workspace:>=5 <6', refused: false, resolves: '>=5 <6' }, - { spec: 'workspace:@allxsmith/bestax-bulma@^5', refused: true }, - { spec: 'workspace:bestax-bulma@^5', refused: true }, - { spec: 'catalog:', refused: true }, - { spec: 'catalog:default', refused: true }, -]; - -// --- the invariant ---------------------------------------------------------- - -for (const { spec, refused } of SHAPES) { - test(`agreement: "${spec}" is ${refused ? 'refused by both' : 'accepted by both'}`, () => { - assert.equal( - packRefuses(spec), - checkFlags(spec), - `pack-manifest.mjs and check-conformance.mjs disagree about "${spec}". ` + - `That is a green-CI/red-release inversion: whichever one is wrong, the ` + - `check will pass a manifest the pack hooks then refuse to publish.` - ); - assert.equal(packRefuses(spec), refused, `expected refusal=${refused}`); - }); -} - -// --- what the accepted shapes actually resolve to --------------------------- - -for (const { spec, refused, resolves } of SHAPES) { - if (refused) continue; - test(`resolve: "${spec}" -> "${resolves}"`, () => { - assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), resolves); - }); -} - -// --- the false-positive guard the #412 fix depends on ----------------------- - -test('an explicit range is not mistaken for the alias form', () => { - // `rest.includes('/') || rest.lastIndexOf('@') > 0` is the alias predicate on - // both sides. If it ever caught `workspace:^5.0.0`, the fix for #412 would - // start rejecting the very shape it was written to support. - for (const spec of [ - 'workspace:^5.0.0', - 'workspace:>=5 <6', - 'workspace:5.0.0', - ]) { - assert.equal(checkFlags(spec), false, `${spec} must not be flagged`); - assert.equal(packRefuses(spec), false, `${spec} must not be refused`); - } -}); - -test('a scoped name in the alias form is caught by the "/" arm', () => { - // Two arms, two shapes: scoped names trip `includes('/')`, unscoped ones trip - // `lastIndexOf('@') > 0`. Both are exercised so neither can be dropped. - assert.ok(packRefuses('workspace:@scope/pkg@^1')); - assert.ok(checkFlags('workspace:@scope/pkg@^1')); - assert.ok(packRefuses('workspace:pkg@^1')); - assert.ok(checkFlags('workspace:pkg@^1')); -}); - -// --- shapes neither side owns ---------------------------------------------- - -test('a plain semver range is nobody’s business', () => { - for (const spec of ['^5.0.0', '5.0.0', '>=5 <6', 'npm:other@^1']) { - assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), null); - assert.equal(checkFlags(spec), false); - } -}); - -test('a non-string specifier is left alone rather than crashing', () => { - // package.json can carry odd values; neither side should throw on them. - for (const spec of [undefined, null, 42, {}]) { - assert.equal(rewriteSpecifier(NAME, spec, resolveVersion), null); - assert.equal(checkFlags(spec), false); - } -}); - -// --- messages name the dependency, since that is what a maintainer greps ---- - -test('a refusal names the offending entry and the specifier', () => { - assert.throws( - () => - rewriteSpecifier(NAME, 'catalog:', resolveVersion, 'devDependencies.x'), - err => - err instanceof UnsupportedSpecifierError && - err.message.includes('devDependencies.x') && - err.message.includes('catalog:') - ); - assert.throws( - () => - rewriteSpecifier( - NAME, - 'workspace:pkg@^1', - resolveVersion, - 'devDependencies.y' - ), - err => - err instanceof UnsupportedSpecifierError && - err.message.includes('devDependencies.y') && - err.message.includes('alias form') - ); -}); - -test('resolveSpecifier defers the version lookup to its caller', () => { - // The seam that makes this testable at all: no pnpm-linked tree required. - const calls = []; - const spy = name => { - calls.push(name); - return '1.2.3'; - }; - assert.equal(resolveSpecifier(NAME, 'workspace:^', spy), '^1.2.3'); - assert.deepEqual(calls, [NAME]); -}); - -// --- what the CLI does with a refusal --------------------------------------- -// -// The specifier decisions above are the invariant this file exists for, but the -// exit CODE is what npm keys off to abort a publish. These refusals used to be -// `process.exit(1)` and are now thrown and translated by `main`, so the -// translation is worth freezing: if a refusal ever returned 0, npm would -// publish the broken tarball this whole guard exists to prevent — the bug -// inverted. - -/** A throwaway package root, so nothing in the repo is touched. */ -function fixtureRoot(pkg) { - const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pack-manifest-')); - fs.writeFileSync( - path.join(root, 'package.json'), - `${JSON.stringify(pkg, null, 2)}\n` - ); - return root; -} - -const readManifest = root => - fs.readFileSync(path.join(root, 'package.json'), 'utf8'); -const backupPath = root => path.join(root, 'package.json.pack-backup'); - -test('a refused specifier aborts the pack without touching the manifest', () => { - const root = fixtureRoot({ - name: 'fixture', - version: '0.0.0', - devDependencies: { '@allxsmith/bestax-bulma': 'catalog:' }, - }); - const before = readManifest(root); - - assert.equal(main(['prepack'], { pkgRoot: root }), 1, 'must exit non-zero'); - assert.equal(readManifest(root), before, 'manifest must be left intact'); - // A stale backup makes the NEXT prepack refuse to run, so a half-done - // refusal would wedge the following release too. - assert.equal(fs.existsSync(backupPath(root)), false, 'no backup left behind'); -}); - -test('the alias form aborts the pack the same way', () => { - const root = fixtureRoot({ - name: 'fixture', - version: '0.0.0', - devDependencies: { 'bestax-bulma': 'workspace:@allxsmith/bestax-bulma@^5' }, - }); - const before = readManifest(root); - assert.equal(main(['prepack'], { pkgRoot: root }), 1); - assert.equal(readManifest(root), before); - assert.equal(fs.existsSync(backupPath(root)), false); -}); - -test('prepack refuses to clobber a backup a previous pack left behind', () => { - const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); - fs.writeFileSync(backupPath(root), '{}'); - assert.equal(main(['prepack'], { pkgRoot: root }), 1); -}); - -test('a manifest with nothing to rewrite is a no-op, not a failure', () => { - const root = fixtureRoot({ - name: 'fixture', - version: '0.0.0', - dependencies: { bulma: '^1.0.4' }, - }); - const before = readManifest(root); - assert.equal(main(['prepack'], { pkgRoot: root }), 0); - assert.equal(readManifest(root), before); - assert.equal(fs.existsSync(backupPath(root)), false); -}); - -test('postpack with no backup is a no-op, not a failure', () => { - const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); - assert.equal(main(['postpack'], { pkgRoot: root }), 0); -}); - -test('an unknown mode fails rather than silently doing nothing', () => { - const root = fixtureRoot({ name: 'fixture', version: '0.0.0' }); - assert.equal(main(['bogus'], { pkgRoot: root }), 1); - assert.equal(main([], { pkgRoot: root }), 1); -}); - -test('prepack rewrites and postpack restores the repo manifest exactly', () => { - // The round trip is the whole safety property: the release commits - // package.json, so a postpack that did not restore byte-for-byte would - // commit resolved specifiers back into the workspace. - const root = fixtureRoot({ - name: 'fixture', - version: '0.0.0', - devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, - }); - const linked = path.join(root, 'node_modules', '@allxsmith', 'bestax-bulma'); - fs.mkdirSync(linked, { recursive: true }); - fs.writeFileSync( - path.join(linked, 'package.json'), - JSON.stringify({ name: '@allxsmith/bestax-bulma', version: '5.11.1' }) - ); - const before = readManifest(root); - - assert.equal(main(['prepack'], { pkgRoot: root }), 0); - const packed = JSON.parse(readManifest(root)); - assert.equal( - packed.devDependencies['@allxsmith/bestax-bulma'], - '^5.11.1', - 'the packed manifest carries a real range' - ); - assert.ok(fs.existsSync(backupPath(root)), 'the original is backed up'); - - assert.equal(main(['postpack'], { pkgRoot: root }), 0); - assert.equal(readManifest(root), before, 'restored byte for byte'); - assert.equal(fs.existsSync(backupPath(root)), false, 'backup cleaned up'); -}); - -test('an unresolvable workspace dep fails instead of packing a broken range', () => { - // No linked package in node_modules — `pnpm install` was never run. Packing - // anyway would emit an empty or bogus specifier. This exits 1 rather than - // propagating, which is what the CLI did before the refactor and what npm - // reads to abort the publish. - const root = fixtureRoot({ - name: 'fixture', - version: '0.0.0', - devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, - }); - const before = readManifest(root); - assert.equal(main(['prepack'], { pkgRoot: root }), 1); - assert.equal(readManifest(root), before); - assert.equal(fs.existsSync(backupPath(root)), false); -}); - -// --- agreement beyond the enumerated shapes --------------------------------- -// -// The table above pins the twelve shapes pnpm documents. Deep review on #531 -// noted the gap that leaves: the two predicates agree on those twelve by -// assertion, and on everything else only because they are currently textually -// identical. An edit to one of them that happens to change a shape nobody -// listed would slip through. -// -// So assert the biconditional over a spread of odd, adversarial and -// not-yet-invented specifiers. This deliberately does NOT assert what the -// verdict should be — only that both sides reach the same one. Deciding the -// right answer for a hypothetical future pnpm syntax is not this test's job; -// noticing that the two files stopped answering it the same way is. -const ODD_SPECIFIERS = [ - // plausible future or undocumented pnpm shapes - 'workspace:^1.0.0-beta.1', - 'workspace:*-next', - 'workspace:latest', - 'workspace:1.x', - 'workspace:>=1', - 'workspace:@scope/name', - 'workspace:@scope/name@', - 'workspace:name@', - 'workspace:@', - 'workspace:@@', - 'workspace://', - 'workspace:a/b/c@1', - 'catalog:with-a-name', - 'catalog:@scope/thing', - // adjacent protocols neither side owns - 'npm:pkg@^1', - 'file:../pkg', - 'link:../pkg', - 'git+https://example.test/x.git', - 'jsr:@scope/pkg', - // degenerate strings - '', - ' ', - 'workspace', - 'workspaces:*', - 'catalog', - 'CATALOG:', - 'WORKSPACE:^', - 'x'.repeat(200), - 'workspace:' + 'a'.repeat(200), -]; - -for (const spec of ODD_SPECIFIERS) { - test(`agreement holds for an unlisted shape: ${JSON.stringify(spec).slice(0, 40)}`, () => { - assert.equal( - packRefuses(spec), - checkFlags(spec), - `pack-manifest.mjs and check-conformance.mjs disagree about ` + - `${JSON.stringify(spec)}. Whichever is right, one of them was edited ` + - `without the other — the drift this file exists to catch.` - ); - }); -} diff --git a/scripts/publishable-manifests.test.mjs b/scripts/publishable-manifests.test.mjs new file mode 100644 index 000000000..4be03c901 --- /dev/null +++ b/scripts/publishable-manifests.test.mjs @@ -0,0 +1,513 @@ +/** + * Holds the `publishable-manifests` rule in scripts/check-conformance.mjs to + * what the repo actually does (#436). + * + * The rule exempts declared packages from part of the pack-time protocol check, + * because they publish with `pnpm publish`, which resolves those protocols. The + * exemption is the dangerous verdict: granted wrongly, it waves through the + * manifest that shipped #412. + * + * So the exemption is DECLARED in check-conformance.mjs rather than inferred + * from release configs, and the checking of that declaration lives here. That + * split is the point. Four separate false exemptions came from a parser that + * modelled semantic-release's config format and fell through to "exempt" + * whenever it met a shape it did not know. Here, a wrong reading fails a test + * instead of switching a rule off. + */ +import { existsSync, readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { + hookScripts, + manifestViolations, + parseWorkspacePackages, +} from './check-conformance.mjs'; +import { execOptions, releaseBranches } from './lib/release-config.mjs'; +import { tokenize } from './lib/shell-words.mjs'; + +const repoFile = rel => + readFileSync(fileURLToPath(new URL(`../${rel}`, import.meta.url)), 'utf8'); + +// --- the declaration matches reality ----------------------------------------- + +/** The declared set, read from the check rather than restated here. */ +const DECLARED = new Set( + [ + ...repoFile('scripts/check-conformance.mjs').matchAll( + /const PNPM_PUBLISHED = new Set\(\[([^\]]*)\]\)/g + ), + ] + .flatMap(m => m[1].split(',')) + .map(s => s.trim().replace(/^['"]|['"]$/g, '')) + .filter(Boolean) +); + +const PUBLISHABLE = parseWorkspacePackages( + repoFile('pnpm-workspace.yaml') +).filter(dir => { + try { + return !JSON.parse(repoFile(`${dir}/package.json`)).private; + } catch { + return false; + } +}); + +test('the declaration was actually parsed out of the check', () => { + // Everything below compares against DECLARED, so an empty read would make + // this whole file vacuous. + assert.ok(DECLARED.size > 0, 'PNPM_PUBLISHED could not be read'); + assert.ok( + PUBLISHABLE.length >= 4, + `expected 4+ publishable packages, got ${PUBLISHABLE.length}` + ); +}); + +/** + * The command a package's release config actually runs, read from the LOADED + * config rather than its source text. + * + * The first version of this grepped the file for `pnpm publish`, on the theory + * that a substring is too dumb to be fooled. It was fooled immediately: the + * release config explains at length why it runs `pnpm publish` with + * `--provenance` and `--embed-readme`, so the prose satisfied every assertion + * and the command itself was unconstrained. Editing the real command to + * `npm publish`, or deleting both flags, left the whole suite green. + * + * Reading one known field of one declared config is not the config-format + * modelling that failed four times — that failed because it inferred a VERDICT + * from shapes it might not recognise, and fell through to "exempt". Here an + * unreadable config throws, which fails a test. + */ +async function publishCommand(dir) { + // A package with no exec plugin publishes with npm, which is a real answer, + // not a missing one. + return (await execOptions(dir))?.publishCmd ?? ''; +} + +for (const dir of PUBLISHABLE) { + test(`${dir}: the release config agrees with the declaration`, async () => { + const cmd = await publishCommand(dir); + const runsPnpmPublish = /(^|\s|&&|\{)\s*pnpm\s+publish(\s|$)/.test(cmd); + if (DECLARED.has(dir)) { + assert.ok( + runsPnpmPublish, + `${dir} is declared in PNPM_PUBLISHED but its publishCmd is ` + + `${JSON.stringify(cmd)}, which does not run pnpm publish. The ` + + `declaration grants it an exemption it has not earned.` + ); + } else { + assert.ok( + !runsPnpmPublish, + `${dir}'s publishCmd runs pnpm publish but ${dir} is not declared in ` + + `PNPM_PUBLISHED, so it is being held to the npm rule. Declare it, ` + + `or remove the command.` + ); + } + }); +} + +test('bestax-migrate is the one declared package, and it wires the guard', () => { + // Stated rather than derived, so adding a package to the declaration is a + // deliberate act that fails this test first. + assert.deepEqual([...DECLARED], ['bestax-migrate']); + const pkg = JSON.parse(repoFile('bestax-migrate/package.json')); + assert.match(pkg.scripts.prepublishOnly, /require-pnpm-publish\.mjs/); +}); + +test('the declared package passes --provenance and --embed-readme', async () => { + // Neither is optional and neither fails loudly if dropped: pnpm ignores + // publishConfig.provenance (and this package no longer carries one), and + // defaults embed-readme to false where npm defaults it true. Asserted + // against the command, not the file: both flags appear in the config's + // comments, so a source grep passed with them deleted from publishCmd. + const cmd = await publishCommand('bestax-migrate'); + assert.match(cmd, /(^|\s)--provenance(\s|$)/); + assert.match(cmd, /(^|\s)--embed-readme(\s|$)/); +}); + +test('the scripts the release config names all exist', async () => { + // The guard the deleted pack-hook block carried. Read from the commands + // through the same tokenizer that has to undo the config's quoting, rather + // than by pattern-matching filenames out of the source — which broke on any + // path shape other than `path.join(SCRIPTS, 'x.mjs')` and invented + // requirements for filenames mentioned in comments. + const exec = await execOptions('bestax-migrate'); + assert.ok(exec, 'bestax-migrate must publish through @semantic-release/exec'); + + // Only the *Cmd options are shell commands. execCwd is a raw, unquoted path, + // and feeding it to tokenize threw on a checkout containing an apostrophe — + // the very case shell-words exists to survive. + const named = Object.entries(exec) + .filter(([key, v]) => key.endsWith('Cmd') && typeof v === 'string') + .flatMap(([, cmd]) => tokenize(cmd)) + .filter(word => /\.(mjs|cjs|js)$/.test(word)); + + assert.ok( + named.length >= 2, + `expected the config to name scripts, got ${named}` + ); + for (const abs of named) { + assert.ok( + existsSync(abs), + `release.config.js runs "${abs}", which does not exist` + ); + } +}); + +test('the release stays on a single branch, or the dist-tag needs revisiting', async () => { + // publishCmd passes no --tag, which is only correct while every release goes + // to `latest`. @semantic-release/npm's get-channel.js maps a channel that is + // a valid semver range to `release-`, and nothing here reimplements + // that. + // + // Read from the loaded config, not the source. This assertion was the third + // source-text grep in this file and the only one left after the other two + // were found matching the config's own comments: a line reading + // `// branches: ['main']` would have satisfied it while the real value was + // ['main', 'next'], which is the direction that publishes a prerelease to + // the stable dist-tag. + assert.deepEqual(await releaseBranches('bestax-migrate'), ['main']); +}); + +// --- the rule ---------------------------------------------------------------- +// +// bestax-migrate is the only package carrying a pack-time specifier and it is +// exempt for that one, so none of these branches executes during a real run. +// Without them, inverting the rule leaves CI green. + +// A declared package must also wire the prepublishOnly guard, so fixtures for +// the pnpm side carry it; otherwise every one of them picks up that violation +// instead of the one under test. +const GUARD = { + prepack: 'node ../scripts/require-pnpm-publish.mjs', + prepublishOnly: 'node ../scripts/require-pnpm-publish.mjs', +}; + +const WS = spec => ({ + scripts: GUARD, + devDependencies: { '@allxsmith/bestax-bulma': spec }, +}); + +// Real directory names, because manifestViolations consults the declaration +// itself. `bulma-ui` is not declared, so it stands for an npm publisher; +// `bestax-migrate` is, so it stands for a pnpm one. A rule that ignored the +// declaration would pass fixtures that carried the verdict as an argument. +const NPM_PKG = 'bulma-ui'; +const PNPM_PKG = 'bestax-migrate'; + +test('an npm publisher is held to every pack-time protocol', () => { + for (const spec of [ + 'workspace:^', + 'catalog:', + 'jsr:@scope/pkg@^1', + 'link:../y', + 'portal:../y', + 'file:../y', + ]) { + const v = manifestViolations(NPM_PKG, WS(spec)); + assert.equal(v.length, 1, `${spec} must be flagged for an npm publisher`); + // `file:` is the one npm genuinely understands, so it is not an + // EUNSUPPORTEDPROTOCOL; its message says what actually goes wrong instead. + assert.match( + v[0], + spec.startsWith('file:') ? /none of their machines/ : /#412/ + ); + } +}); + +test('a pnpm publisher is exempt only for the protocols pnpm resolves', () => { + for (const spec of ['workspace:^', 'catalog:', 'catalog:default']) { + assert.deepEqual(manifestViolations(PNPM_PKG, WS(spec)), [], spec); + } + for (const spec of [ + 'jsr:@scope/pkg@^1', + 'link:../y', + 'portal:../y', + 'file:../y', + ]) { + const v = manifestViolations(PNPM_PKG, WS(spec)); + assert.equal(v.length, 1, `${spec} must still be flagged`); + // These do not fail as EUNSUPPORTEDPROTOCOL, so the message must not say so. + assert.doesNotMatch(v[0], /EUNSUPPORTEDPROTOCOL/); + } +}); + +test('a pnpm publisher is exempt only in devDependencies', () => { + for (const section of [ + 'dependencies', + 'peerDependencies', + 'optionalDependencies', + ]) { + const v = manifestViolations(PNPM_PKG, { + scripts: GUARD, + [section]: { x: 'workspace:^' }, + }); + assert.equal(v.length, 1, `${section} must be flagged`); + assert.match(v[0], /resolved by consumers/); + // And must not tell a pnpm publisher to switch to pnpm publish. + assert.doesNotMatch(v[0], /move bestax-migrate to `pnpm publish`/); + } +}); + +test('each message explains the failure that actually applies', () => { + const npm = manifestViolations(NPM_PKG, WS('workspace:^'))[0]; + const jsr = manifestViolations(PNPM_PKG, { + scripts: GUARD, + dependencies: { x: 'jsr:@s/p@^1' }, + })[0]; + const consumer = manifestViolations(PNPM_PKG, { + scripts: GUARD, + dependencies: { x: 'workspace:^' }, + })[0]; + assert.match(npm, /npm publish/); + assert.match(jsr, /@jsr registry/); + assert.match(consumer, /resolved by consumers/); + // Three distinct explanations, not one shared tail re-deriving the predicate. + assert.notEqual(npm, jsr); + assert.notEqual(jsr, consumer); +}); + +test('a plain semver range is nobody’s business', () => { + const clean = { + scripts: GUARD, + dependencies: { bulma: '^1.0.4' }, + devDependencies: { jest: '^30' }, + }; + for (const dir of [NPM_PKG, PNPM_PKG]) { + assert.deepEqual(manifestViolations(dir, clean), []); + } +}); + +test('a non-string specifier does not crash the rule', () => { + for (const spec of [undefined, null, 42, {}]) { + assert.deepEqual( + manifestViolations(NPM_PKG, { dependencies: { x: spec } }), + [] + ); + } +}); + +test('a private package is not held to any of this', () => { + assert.deepEqual( + manifestViolations('docs', { private: true, ...WS('workspace:^') }), + [] + ); +}); + +// --- lifecycle hook script paths --------------------------------------------- + +test('hookScripts collects paths from pack and publish hooks only', () => { + const found = hookScripts({ + scripts: { + prepublishOnly: 'node ../scripts/guard.mjs', + prepack: 'node scripts/a.mjs', + start: 'node dist/index.js', + test: 'node ./tools/t.js', + }, + }); + assert.deepEqual(found.sort(), ['../scripts/guard.mjs', 'scripts/a.mjs']); +}); + +test('hookScripts recognises interpreters other than node', () => { + // `tsx ./x.ts` and `bash ./x.sh` name a script exactly as much as node does. + assert.deepEqual( + hookScripts({ scripts: { prepack: 'tsx ./scripts/stamp.ts' } }), + ['./scripts/stamp.ts'] + ); + assert.deepEqual( + hookScripts({ scripts: { postpack: 'bash ./scripts/g.sh' } }), + ['./scripts/g.sh'] + ); +}); + +test('hookScripts ignores flags and bare filenames', () => { + // A build output is not a script to demand exists: this check runs before the + // build in ci.yml. + assert.deepEqual( + hookScripts({ + scripts: { + prepack: 'node ./scripts/x.mjs --out=bundle.js --require=./p.js', + }, + }), + ['./scripts/x.mjs'] + ); +}); + +test('hookScripts survives a quoted path with a space', () => { + assert.deepEqual( + hookScripts({ scripts: { prepack: `node '/My Projects/x/a.mjs'` } }), + ['/My Projects/x/a.mjs'] + ); +}); + +test('hookScripts tolerates a manifest with no scripts', () => { + assert.deepEqual(hookScripts({}), []); + assert.deepEqual(hookScripts(undefined), []); +}); + +test('an undeclared package gets no exemption, whatever the walk does', () => { + // The mutation this exists for: a walk that hands every package the pnpm + // verdict. Because manifestViolations consults the declaration itself, a + // package that is not in it cannot be exempted from anywhere. + const v = manifestViolations('bulma-ui', WS('workspace:^')); + assert.equal(v.length, 1, 'bulma-ui is not declared and must not be exempt'); + assert.match(v[0], /npm publish/); + // …and the declared one still is. + assert.deepEqual(manifestViolations('bestax-migrate', WS('workspace:^')), []); +}); + +test('a declared package that drops the guard is flagged for it', () => { + // The exemption and its compensating guard are checked together, so a package + // cannot gain one and lose the other in a single edit. + const v = manifestViolations(PNPM_PKG, { + devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, + }); + assert.equal(v.length, 1); + assert.match(v[0], /prepublishOnly/); + assert.match(v[0], /require-pnpm-publish\.mjs/); +}); + +test('an undeclared package is not asked for the guard', () => { + // bulma-ui has no exemption, so it has nothing to compensate for. + const v = manifestViolations(NPM_PKG, { dependencies: { bulma: '^1.0.4' } }); + assert.deepEqual(v, []); +}); + +test('the exec plugin pins its cwd', async () => { + // `pnpm publish` resolves its target package from the cwd, which for exec is + // wherever semantic-release was started. Without execCwd a run from the repo + // root reaches the publish step — after the release commit and tag are + // pushed — and fails on the private root package. Every other exec option is + // pinned by a test; this one was not. + const exec = await execOptions('bestax-migrate'); + assert.ok(exec, 'bestax-migrate must publish through @semantic-release/exec'); + assert.ok(exec.execCwd, 'execCwd must be set'); + assert.match(exec.execCwd, /bestax-migrate$/); +}); + +test('a violation names a fix that fits the protocol', () => { + // Both halves of the npm message have to match. `file:` is not an + // EUNSUPPORTEDPROTOCOL, and suggesting a pnpm migration for a protocol pnpm + // does not resolve sends the maintainer through a migration that lands on + // the same specifier. + const ws = manifestViolations(NPM_PKG, WS('workspace:^'))[0]; + assert.match(ws, /EUNSUPPORTEDPROTOCOL/); + assert.match(ws, /PNPM_PUBLISHED/); + + const file = manifestViolations(NPM_PKG, WS('file:../y'))[0]; + assert.doesNotMatch(file, /EUNSUPPORTEDPROTOCOL/); + assert.doesNotMatch(file, /PNPM_PUBLISHED/); + assert.match(file, /does not resolve it either/); + + for (const spec of ['jsr:@s/p@^1', 'link:../y', 'portal:../y']) { + assert.doesNotMatch( + manifestViolations(NPM_PKG, WS(spec))[0], + /PNPM_PUBLISHED/, + `${spec} must not be advertised as fixable by moving to pnpm publish` + ); + } +}); + +test('the suggested prepublishOnly path fits the package depth', () => { + // Hardcoding `../scripts/…` is only right for a package one level down. + const v = manifestViolations(PNPM_PKG, { + devDependencies: { '@allxsmith/bestax-bulma': 'workspace:^' }, + })[0]; + assert.match(v, /node \.\.\/scripts\/require-pnpm-publish\.mjs/); +}); + +test('hookScripts skips paths it cannot resolve rather than inventing them', () => { + // A shell variable cannot be expanded here, and access()ing the literal text + // would report a working hook as broken. + assert.deepEqual( + hookScripts({ scripts: { prepack: 'node $INIT_CWD/scripts/a.mjs' } }), + [] + ); + // An unbalanced quote is a command the shell would reject outright. + assert.deepEqual(hookScripts({ scripts: { prepack: `node './x.mjs` } }), []); +}); + +test('hookScripts sees every script in a chained hook', () => { + // `;`, `|` and `>` end a word without whitespace, so a path abutting one was + // previously missed — the direction that lets a moved script through. + assert.deepEqual( + hookScripts({ + scripts: { prepack: 'node ./scripts/a.mjs;node ./scripts/b.mjs' }, + }).sort(), + ['./scripts/a.mjs', './scripts/b.mjs'] + ); + assert.deepEqual( + hookScripts({ scripts: { postpack: 'node ./scripts/a.mjs|tee log' } }), + ['./scripts/a.mjs'] + ); +}); + +test('a peer dependency is told to pin a range, not to move', () => { + // A peer dep is meant to reach consumers, so "move it to devDependencies" + // would break the contract rather than fix the specifier. + const peer = manifestViolations(PNPM_PKG, { + scripts: GUARD, + peerDependencies: { x: 'workspace:^' }, + })[0]; + assert.match(peer, /semver range/); + assert.doesNotMatch(peer, /Move it to devDependencies/); + + // A runtime dependency still gets the move suggestion. + const runtime = manifestViolations(PNPM_PKG, { + scripts: GUARD, + dependencies: { x: 'workspace:^' }, + })[0]; + assert.match(runtime, /Move it to devDependencies/); +}); + +test('the guard must be run, not merely mentioned', () => { + // Both bypasses that satisfied a substring test: naming the file in another + // command, and short-circuiting past it. + const flagged = scripts => + manifestViolations(PNPM_PKG, { scripts }).some(v => /does not run/.test(v)); + + const REAL = 'node ../scripts/require-pnpm-publish.mjs'; + assert.equal(flagged({ prepack: REAL, prepublishOnly: REAL }), false); + assert.equal( + flagged({ prepack: REAL, prepublishOnly: 'echo require-pnpm-publish.mjs' }), + true, + 'a mention must not satisfy the check' + ); + assert.equal( + flagged({ prepack: REAL, prepublishOnly: `true || ${REAL}` }), + true, + 'short-circuiting past the guard must not satisfy the check' + ); + assert.equal( + flagged({ prepack: REAL, prepublishOnly: `${REAL} && echo ok` }), + false, + 'chaining after the guard is fine' + ); +}); + +test('both pack hooks are required, since npm pack runs only prepack', () => { + // `npm pack` never runs prepublishOnly, and `npm publish ` runs no + // scripts at all, so prepublishOnly alone leaves a two-step hand publish + // shipping the unresolved specifier. + const REAL = 'node ../scripts/require-pnpm-publish.mjs'; + const only = manifestViolations(PNPM_PKG, { + scripts: { prepublishOnly: REAL }, + }); + assert.equal(only.length, 1); + assert.match(only[0], /prepack/); +}); + +test('an unresolvable protocol in devDependencies is explained honestly', () => { + // Consumers never resolve a dependency's devDependencies, so the + // consumer-facing complaint does not apply there — and "give it a semver + // range" is not a possible fix for a local path. + const v = manifestViolations(PNPM_PKG, { + scripts: GUARD, + devDependencies: { x: 'file:../fixtures' }, + })[0]; + assert.match(v, /consumers do not resolve devDependencies/); + assert.doesNotMatch(v, /no consumer can resolve it/); + assert.doesNotMatch(v, /plain semver range/); +}); diff --git a/scripts/require-pnpm-publish.mjs b/scripts/require-pnpm-publish.mjs new file mode 100644 index 000000000..af9bf4abb --- /dev/null +++ b/scripts/require-pnpm-publish.mjs @@ -0,0 +1,134 @@ +#!/usr/bin/env node +/** + * Refuses a publish driven by anything other than pnpm (#436). + * + * bestax-migrate keeps `"@allxsmith/bestax-bulma": "workspace:^"` in + * devDependencies. `pnpm publish` rewrites that to a real range at pack time; + * `npm publish` ships the protocol verbatim, which is what made 1.0.0 + * uninstallable (#412). + * + * That used to be covered twice over: a `prepack` hook rewrote the specifier + * for whatever was packing, and `check:conformance` refused to let the + * specifier exist without those hooks wired. #436 removed the hook, because + * hand-rolling pnpm's rewrite is the bug class it exists to stop owning, and + * the conformance rule now EXEMPTS this package precisely because pnpm handles + * it. Both of those are right, and together they left the specifier with no + * mechanical guard at all: correct in the release pipeline, and a prose rule in + * CLAUDE.md everywhere else. + * + * So guard the packer instead of the specifier. `pnpm publish` runs + * `prepublishOnly` (verified against pnpm 11.9.0, which invokes it alongside + * `prepublish` before packing), and so does `npm publish`, so this hook sees + * both and can tell them apart by the user agent each sets: + * + * pnpm/11.9.0 npm/? node/v25.2.1 darwin arm64 + * npm/11.6.2 node/v25.2.1 darwin arm64 workspaces/false + * + * Scope, stated plainly rather than implied. bestax-migrate wires this to BOTH + * `prepack` and `prepublishOnly`, which between them cover `npm publish` and + * `npm pack` — the latter matters because `npm publish ` runs no + * scripts at all, so a tarball packed by npm could otherwise be published with + * nothing left to refuse it. What it does NOT cover: + * + * - `--ignore-scripts`, which skips these hooks entirely. Both npm and pnpm + * gate lifecycle scripts on it (pnpm 11.9.0 wraps the `prepublishOnly` / + * `prepublish` call in `if (!opts.ignoreScripts)`), so `npm publish + * --ignore-scripts` ships the unresolved specifier with no signal at all. + * Worth naming rather than leaving implied, because a repo whose + * supply-chain policy is built on blocking install scripts is exactly the + * kind of place that reaches for that flag out of habit. + * - a tarball packed before this guard existed, or packed elsewhere. + * + * A hook cannot cover those, so this is a guard against the likely mistake, not + * a proof. Inspect the tarball with `pnpm -C bestax-migrate pack` if you are + * unsure what a manifest will ship. + */ +import process from 'node:process'; +import { pathToFileURL } from 'node:url'; + +/** + * Keyed on `npm_execpath`, NOT on `npm_config_user_agent`. + * + * The user agent is inherited. npm relays whatever it finds in the + * environment, so `pnpm exec npm publish` runs this hook reporting + * `pnpm/11.9.0 …` and an agent check waves it straight through — while npm, + * not pnpm, assembles the tarball and ships `workspace:^` unresolved. That is + * #412 through the guard written to stop it, via the most natural + * hand-publish form in a pnpm monorepo. Measured: + * + * pnpm publish agent pnpm/… execpath …/pnpm/11.9.0/bin/pnpm.mjs + * npm publish agent npm/… execpath …/npm/bin/npm-cli.js + * pnpm exec npm publish agent pnpm/… execpath …/npm/bin/npm-cli.js + * + * `npm_execpath` is rewritten by whichever process actually runs the lifecycle + * script, so it names the real packer where the agent only names an ancestor. + * + * An ABSENT execpath is treated as allowed: `prepublishOnly` only runs under a + * package manager, so nothing there means the script was invoked directly, and + * failing would be a confusing refusal rather than a caught mistake. + */ +export function isPnpmPublish(execPath) { + if (!execPath) return true; + const binary = String(execPath).trim().split(/[\\/]/).pop() ?? ''; + const name = binary.replace(/\.(c|m)?js$|\.cmd$|\.exe$|\.bat$|\.ps1$/i, ''); + + // Refuse only what is POSITIVELY a different packer. Anything unrecognised is + // allowed, and that asymmetry is deliberate: refusing an unknown value fails + // a real release from inside a pack hook, after semantic-release has pushed + // the commit and tag, and pnpm's own lifecycle runner can hand us one. + // Verified in pnpm 11.9.0's bundle: + // + // env.npm_execpath = process.pkg != null + // ? process.execPath + // : process.argv[1] || process.cwd() + // + // so a build where argv[1] is falsy reports the package DIRECTORY. Under a + // known-good-only rule that reads as "not pnpm" and kills the release; here + // it reads as unrecognised and passes, while `pnpm exec npm publish` — the + // case this guard exists for — still names npm-cli and is still refused. + const OTHER_PACKAGE_MANAGERS = + /^(npm|npm-cli|npx|yarn|yarnpkg|bun|bunx|cnpm|tnpm)$/i; + return !OTHER_PACKAGE_MANAGERS.test(name); +} + +export function main(env = process.env, log = console.error) { + if (isPnpmPublish(env.npm_execpath)) { + // A hand-run `pnpm publish` is allowed, and silently produces neither + // provenance nor an embedded README: those flags live in the release + // config's publishCmd, and this package deliberately carries no + // publishConfig.provenance for pnpm to fall back on. CI passes them, so say + // nothing there; a human gets one line before the tarball goes out. + if (!env.CI && !env.GITHUB_ACTIONS) { + log( + 'require-pnpm-publish: publishing by hand. `--provenance ' + + '--embed-readme` are not defaults here and CI passes them for you; ' + + 'without them this release ships unattested and its npm page loses ' + + 'its README.' + ); + } + return 0; + } + log( + 'This package must be published with `pnpm publish`, not ' + + `\`npm publish\` (packer: ${env.npm_execpath}).\n` + + '\n' + + 'It declares "@allxsmith/bestax-bulma": "workspace:^" in ' + + 'devDependencies. pnpm resolves the workspace: protocol at pack time; ' + + 'npm ships it verbatim, and the published package is then uninstallable ' + + 'for everyone (EUNSUPPORTEDPROTOCOL, #412 shipped exactly this as ' + + '1.0.0).\n' + + '\n' + + 'Releases are automated and run from CI. If you really are publishing ' + + 'by hand, the flags are not optional either, because this package no ' + + 'longer carries a publishConfig.provenance for pnpm to read:\n' + + '\n' + + ' pnpm publish --provenance --embed-readme --access public\n' + + '\n' + + 'Check what it would ship first with `pnpm -C bestax-migrate pack`.' + ); + return 1; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + process.exitCode = main(); +} diff --git a/scripts/require-pnpm-publish.test.mjs b/scripts/require-pnpm-publish.test.mjs new file mode 100644 index 000000000..ad9657767 --- /dev/null +++ b/scripts/require-pnpm-publish.test.mjs @@ -0,0 +1,141 @@ +/** + * Covers scripts/require-pnpm-publish.mjs (#436). + * + * Both directions are load-bearing and they fail differently. A guard that + * misses `npm publish` lets #412 ship again with no signal. A guard that + * refuses `pnpm publish` breaks the actual release, and it would do so during + * `prepublishOnly`, after semantic-release has already pushed the release + * commit and tag. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { isPnpmPublish, main } from './require-pnpm-publish.mjs'; + +const PNPM = '/Users/x/.cache/node/corepack/v1/pnpm/11.9.0/bin/pnpm.mjs'; +const NPM = '/opt/homebrew/lib/node_modules/npm/bin/npm-cli.js'; +const silent = () => {}; + +test('pnpm publish is allowed through', () => { + assert.equal(isPnpmPublish(PNPM), true); + assert.equal(main({ npm_execpath: PNPM }, silent), 0); +}); + +test('npm publish is refused', () => { + assert.equal(isPnpmPublish(NPM), false); + assert.equal(main({ npm_execpath: NPM }, silent), 1); +}); + +test('another package manager is refused too', () => { + // Only pnpm resolves the workspace: protocol at pack time, so the rule is + // "pnpm or nothing", not "anything but npm". + for (const ua of ['/usr/local/bin/yarn.js', '/usr/bin/bun', '/x/cnpm.js']) { + assert.equal(isPnpmPublish(ua), false, `${ua} must be refused`); + assert.equal(main({ npm_execpath: ua }, silent), 1); + } +}); + +test('an unrecognised packer is allowed, not refused', () => { + // The asymmetry is deliberate. pnpm's own lifecycle runner falls back to + // `process.argv[1] || process.cwd()` for npm_execpath, so a build where + // argv[1] is falsy reports the package DIRECTORY. Refusing what we do not + // recognise would kill that release from inside a pack hook, after the + // commit and tag are pushed. + for (const p of [ + '/home/runner/work/bestax/bestax-migrate', + '/bin/pnpmx.mjs', + '/opt/some-future-manager', + ]) { + assert.equal(isPnpmPublish(p), true, `${p} is unrecognised and must pass`); + } + // …while every packer we can name is still refused. + for (const p of [ + '/x/npm-cli.js', + '/x/yarn.js', + '/usr/bin/bun', + '/x/cnpm.js', + ]) { + assert.equal(isPnpmPublish(p), false, `${p} must be refused`); + } +}); + +test('the user agent is NOT the signal, because npm relays it', () => { + // The bug this replaced: `pnpm exec npm publish` runs the hook with the + // inherited agent `pnpm/11.9.0 …` while npm assembles the tarball. Measured + // in this repo; the guard allowed it. execpath is rewritten by whichever + // process actually runs the script, so it names the real packer. + assert.equal( + main( + { + npm_config_user_agent: 'pnpm/11.9.0 npm/? node/v25.2.1 darwin arm64', + npm_execpath: NPM, + }, + silent + ), + 1, + 'a pnpm agent with an npm execpath is `pnpm exec npm publish` and must be refused' + ); +}); + +test('a windows pnpm is allowed, in every form it ships as', () => { + // pnpm is pnpm.cmd or pnpm.exe on Windows. Refusing those blocks a real + // release from inside prepublishOnly, after the commit and tag are pushed — + // the one direction this guard must not fail in. + for (const p of [ + 'C:\\Users\\x\\pnpm.cmd', + 'C:\\Users\\x\\pnpm.exe', + 'C:\\Users\\x\\pnpm.CJS', + 'C:\\Users\\x\\pnpm.cjs', + 'C:\\Users\\x\\pnpm.bat', + 'C:\\Users\\x\\pnpm.ps1', + ]) { + assert.equal(isPnpmPublish(p), true, `${p} is pnpm and must be allowed`); + } + for (const p of [ + 'C:\\Program Files\\nodejs\\npm-cli.js', + 'C:\\Users\\x\\npm.cmd', + 'C:\\Users\\x\\yarn.cmd', + ]) { + assert.equal(isPnpmPublish(p), false, `${p} is not pnpm`); + } +}); + +test('the refusal spells out the flags a hand publish would otherwise lose', () => { + // publishConfig.provenance was removed from the manifest, so a hand + // `pnpm publish` — the one path this guard permits — produces no provenance + // and no embedded README unless the flags are passed. + let msg = ''; + main({ npm_execpath: NPM }, m => (msg = m)); + assert.match(msg, /--provenance/); + assert.match(msg, /--embed-readme/); +}); + +test('an absent execpath is allowed, not refused', () => { + // prepublishOnly only runs under a package manager, so no agent means the + // script was invoked directly. Failing there would be a confusing refusal + // rather than a caught mistake. + assert.equal(isPnpmPublish(undefined), true); + assert.equal(isPnpmPublish(''), true); + assert.equal(main({}, silent), 0); +}); + +test('the refusal explains the consequence, not just the rule', () => { + let msg = ''; + main({ npm_execpath: NPM }, m => (msg = m)); + assert.match(msg, /pnpm publish/); + assert.match(msg, /workspace:/); + assert.match(msg, /#412/); + assert.match(msg, /EUNSUPPORTEDPROTOCOL/); +}); + +test('bestax-migrate actually wires the hook up', async () => { + // The script existing is not the same as it running. + const pkg = await import('../bestax-migrate/package.json', { + with: { type: 'json' }, + }); + assert.match( + pkg.default.scripts.prepublishOnly, + /require-pnpm-publish\.mjs/, + 'bestax-migrate must run the guard on prepublishOnly' + ); +}); diff --git a/scripts/shell-words.test.mjs b/scripts/shell-words.test.mjs new file mode 100644 index 000000000..16d378d68 --- /dev/null +++ b/scripts/shell-words.test.mjs @@ -0,0 +1,124 @@ +/** + * Holds scripts/lib/shell-words.mjs's two halves in agreement (#436). + * + * `quote` writes the release commands; `tokenize` reads them back to find the + * script paths they name. A value that survives quoting but not tokenizing + * makes check:conformance report a missing script on a config that would + * publish fine. The round trip is the property, so it is asserted over the + * awkward cases directly rather than left to agree by inspection. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { quote, tokenize } from './lib/shell-words.mjs'; + +const AWKWARD = [ + '/plain/path/script.mjs', + '/My Projects/bestax/scripts/x.mjs', + "/Users/o'brien/bestax/scripts/x.mjs", + '/path/with"double/x.mjs', + '/path/with$dollar/x.mjs', + '/path/with\\backslash/x.mjs', + '/trailing space /x.mjs', + '/-leading-dash/x.mjs', +]; + +for (const value of AWKWARD) { + test(`round trip: ${value}`, () => { + // One word in, one word out, unchanged. + assert.deepEqual(tokenize(quote(value)), [value]); + }); +} + +test('a quoted value survives inside a larger command', () => { + const path = "/Users/o'brien/My Projects/x.mjs"; + const cmd = `node ${quote(path)} --dir=${quote('/A B')} \${nextRelease.version}`; + assert.deepEqual(tokenize(cmd), [ + 'node', + path, + '--dir=/A B', + '${nextRelease.version}', + ]); +}); + +test('an unquoted command tokenizes on whitespace', () => { + assert.deepEqual(tokenize('pnpm publish --no-git-checks --provenance'), [ + 'pnpm', + 'publish', + '--no-git-checks', + '--provenance', + ]); +}); + +test('operators never stay glued to a path', () => { + // The property callers depend on: a path abutting an operator is still its + // own word. Operators split character by character, which is coarser than a + // real shell parser and deliberately so — nothing here interprets them, it + // only needs them not to swallow a filename. + assert.deepEqual(tokenize('a 1>&2 && { b || true; }'), [ + 'a', + '1', + '>', + '&', + '2', + '&', + '&', + '{', + 'b', + '|', + '|', + 'true', + ';', + '}', + ]); + assert.ok(tokenize('node ./a.mjs;node ./b.mjs').includes('./a.mjs')); + assert.ok(tokenize('node ./a.mjs>out').includes('./a.mjs')); +}); + +test('an empty or whitespace-only command yields no words', () => { + assert.deepEqual(tokenize(''), []); + assert.deepEqual(tokenize(' '), []); +}); + +test('an empty quoted string is a real, empty word', () => { + assert.deepEqual(tokenize("a '' b"), ['a', '', 'b']); +}); + +test('shell operators end a word even without whitespace', () => { + // `node ./a.mjs;node ./b.mjs` names two scripts, and a caller scanning for + // paths has to see both. + assert.deepEqual(tokenize('node ./a.mjs;node ./b.mjs'), [ + 'node', + './a.mjs', + ';', + 'node', + './b.mjs', + ]); + assert.deepEqual(tokenize('a>b'), ['a', '>', 'b']); + assert.deepEqual(tokenize('a|b'), ['a', '|', 'b']); +}); + +test('backslash escapes are honoured where sh honours them', () => { + // Inside double quotes sh unescapes \" and \; inside single quotes nothing + // is special. + assert.deepEqual(tokenize('node "a\\"b.mjs"'), ['node', 'a"b.mjs']); + assert.deepEqual(tokenize("node 'a\\b.mjs'"), ['node', 'a\\b.mjs']); +}); + +test('an unbalanced quote throws instead of inventing a word', () => { + // The shell would reject the command outright, so accepting it would let a + // caller assert things about a command that cannot run. + assert.throws(() => tokenize("node 'x.mjs"), /unbalanced single quote/); + assert.throws(() => tokenize('node "x.mjs'), /unbalanced double quote/); +}); + +test('subshell parens and backticks end a word', () => { + // `(cd x && node ./a.mjs)` otherwise yields `./a.mjs)`, which no extension + // test matches, so a path scanner drops it silently. + assert.ok( + tokenize('(cd sub && node ./scripts/stamp.mjs)').includes( + './scripts/stamp.mjs' + ) + ); + assert.ok(tokenize('node `which x`/a.mjs').includes('/a.mjs')); +}); diff --git a/scripts/verify-oidc-context.mjs b/scripts/verify-oidc-context.mjs new file mode 100644 index 000000000..cda8e4939 --- /dev/null +++ b/scripts/verify-oidc-context.mjs @@ -0,0 +1,108 @@ +#!/usr/bin/env node +/** + * Pre-flight for the packages that publish with `pnpm publish` (#436). + * + * bestax-migrate hands its publish step to `@semantic-release/exec` running + * `pnpm publish`, because `npm publish` does not resolve pnpm's `workspace:` + * protocol and shipped an uninstallable 1.0.0 (#412). Doing that costs one + * thing worth replacing: `@semantic-release/npm` performed a real OIDC token + * exchange inside `verifyConditions`, so a job missing `id-token: write` failed + * before semantic-release had written anything. With `npmPublish: false` that + * check is switched off entirely. + * + * That matters because of the order semantic-release runs things in: EVERY + * `prepare` step (changelog, version bump, `@semantic-release/git`'s commit and + * tag) completes before ANY `publish` step. An auth failure discovered at + * publish time therefore leaves a release commit and a tag on main with no + * package behind them, and that version number is spent. + * + * So assert the cheap half early. This checks that a GitHub Actions OIDC + * context EXISTS — nothing more. It does not mint a token, does not contact the + * registry, and does not establish that npm will accept this repository as a + * trusted publisher for the package. + * + * What earns it its place is purely the TIMING, not the check. pnpm vendors + * libnpmpublish, whose `ensureProvenanceGeneration` already throws EUSAGE on a + * missing ACTIONS_ID_TOKEN_REQUEST_URL — the same variable, the same verdict. + * But it throws during `publish`, which is after the release commit and tag + * have been pushed. Reaching the same conclusion during `verifyConditions` + * costs nothing and leaves the repository untouched. + * + * The residual risk this does NOT cover — the registry rejecting an otherwise + * well-formed token — is only knowable from a real release. It is tolerable + * because pnpm's OIDC failure path warns and falls back to configured + * credentials, of which the publish job deliberately has none: the fallback is + * a hard auth error, not a quiet unsigned publish. + * + * Outside CI this is a no-op — a maintainer running `semantic-release --dry-run` + * locally has no OIDC context and is not about to publish. + */ +import process from 'node:process'; +import { pathToFileURL } from 'node:url'; + +const VARS = ['ACTIONS_ID_TOKEN_REQUEST_URL', 'ACTIONS_ID_TOKEN_REQUEST_TOKEN']; + +/** + * Keyed on GITHUB_ACTIONS rather than CI, and the difference is which way this + * fails when the signal is absent. + * + * `CI` is set by convention, not by contract: a container image, a composite + * action, or a job-level `env:` can leave it unset, and keying on it made this + * guard a silent no-op in exactly that case. Wrong direction for something + * whose whole job is to fail early, since the fallback is discovering the + * problem at publish time with the tag already pushed. + * + * GITHUB_ACTIONS is guaranteed by the runner, and it is what pnpm's own + * `ensureProvenanceGeneration` keys on when deciding whether to demand an OIDC + * context. Using the same signal means this cannot disagree with the thing it + * front-runs. Its value is the string 'true', so compare it. + */ +const inCI = env => String(env.GITHUB_ACTIONS).toLowerCase() === 'true'; + +export function checkOidcContext(env = process.env, { dryRun = false } = {}) { + // semantic-release marks verifyConditions `dryRun: true`, so this runs on a + // dry run too. A dry run never publishes and needs no token, and failing one + // would break the command CONTRIBUTING.md advertises as safe. + if (dryRun) return { ok: true, skipped: true }; + if (!inCI(env)) return { ok: true, skipped: true }; + const missing = VARS.filter(name => !env[name]); + return { ok: missing.length === 0, skipped: false, missing }; +} + +export function main( + env = process.env, + log = console.log, + argv = process.argv.slice(2) +) { + // Printed on stdout rather than stderr, but NOT for the reason it is tempting + // to write down. @semantic-release/exec looks like it builds its + // SemanticReleaseError message from the command's stdout, and its own code + // says so — but the test is `error.stdout.trim.length > 0`, and + // `String.prototype.trim.length` is the function's arity, 0. So `0 > 0` is + // always false and that branch is dead in 7.1.0: every exec failure surfaces + // as `${error.name}: ${error.message}`. + // + // What actually carries this text is exec piping both streams to the job log, + // which works either way. stdout is kept because it is where the explanation + // WOULD be quoted if upstream fixes that typo, and because the exit code, not + // the stream, is what fails the step. + const { ok, skipped, missing } = checkOidcContext(env, { + dryRun: argv.includes('--dry-run'), + }); + if (skipped || ok) return 0; + log( + `verify-oidc-context: ${missing.join(' and ')} ${ + missing.length > 1 ? 'are' : 'is' + } unset, so this job has no OIDC context and ` + + '`pnpm publish --provenance` has nothing to authenticate with.\n' + + 'The usual cause is a publish job that lost `permissions: id-token: ' + + 'write`.\n' + + 'This check only proves the context exists — not that npm will accept ' + + 'the token for this package.' + ); + return 1; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + process.exitCode = main(); +} diff --git a/scripts/verify-oidc-context.test.mjs b/scripts/verify-oidc-context.test.mjs new file mode 100644 index 000000000..5f11c16de --- /dev/null +++ b/scripts/verify-oidc-context.test.mjs @@ -0,0 +1,88 @@ +/** + * Covers scripts/verify-oidc-context.mjs (#436). + * + * This guard exists because moving bestax-migrate to `pnpm publish` switched + * off `@semantic-release/npm`'s OIDC check in verifyConditions, and + * semantic-release pushes the release commit and tag before any publish step + * runs. Both directions are worth pinning: a guard that never fires is + * decoration, and one that fires locally would block `--dry-run` on a + * maintainer's laptop for no reason. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { checkOidcContext, main } from './verify-oidc-context.mjs'; + +const OIDC = { + GITHUB_ACTIONS: 'true', + ACTIONS_ID_TOKEN_REQUEST_URL: 'https://example.test/token', + ACTIONS_ID_TOKEN_REQUEST_TOKEN: 'deadbeef', +}; + +const silent = () => {}; + +test('a complete OIDC context passes', () => { + assert.deepEqual(checkOidcContext(OIDC), { + ok: true, + skipped: false, + missing: [], + }); + assert.equal(main(OIDC, silent), 0); +}); + +test('either variable missing on Actions fails, and the exit code says so', () => { + // npm keys off the exit code to abort; a guard that reported the problem and + // returned 0 would be worse than none, because the log would look checked. + for (const name of [ + 'ACTIONS_ID_TOKEN_REQUEST_URL', + 'ACTIONS_ID_TOKEN_REQUEST_TOKEN', + ]) { + const env = { ...OIDC }; + delete env[name]; + assert.equal(checkOidcContext(env).ok, false); + assert.deepEqual(checkOidcContext(env).missing, [name]); + assert.equal(main(env, silent), 1, `${name} unset must exit 1`); + } + assert.equal( + main({ GITHUB_ACTIONS: 'true' }, silent), + 1, + 'neither set must exit 1' + ); +}); + +test('outside Actions it is a no-op, so a local dry run is not blocked', () => { + assert.deepEqual(checkOidcContext({}), { ok: true, skipped: true }); + assert.equal(main({}, silent), 0); + // Even with the variables half-present, which is what a stray shell export + // looks like. + assert.equal(main({ ACTIONS_ID_TOKEN_REQUEST_URL: 'x' }, silent), 0); +}); + +test('it does not key on CI, which is convention rather than contract', () => { + // Keying on CI made this a silent no-op wherever CI happened to be unset — + // a container image, a composite action, a job-level env: override — which + // is the one direction a fail-early guard must not fail in. GITHUB_ACTIONS + // is guaranteed by the runner and is what pnpm's own provenance check reads. + assert.equal( + checkOidcContext({ CI: 'true' }).skipped, + true, + 'CI alone is not the signal' + ); + assert.equal( + checkOidcContext({ GITHUB_ACTIONS: 'true' }).skipped, + false, + 'GITHUB_ACTIONS alone must arm it, with CI unset' + ); +}); + +test('the failure names the variables and does not overclaim', () => { + // The claim has to match the mechanism: this proves a context exists, not + // that npm will accept the token. A message that implied otherwise would + // stop the next reader checking. + let message = ''; + main({ GITHUB_ACTIONS: 'true' }, m => (message = m)); + assert.match(message, /ACTIONS_ID_TOKEN_REQUEST_URL/); + assert.match(message, /ACTIONS_ID_TOKEN_REQUEST_TOKEN/); + assert.match(message, /id-token: write/); + assert.match(message, /only proves the context exists/); +});