From b91d981a1ca727cd21fc6eb26e83887db9941934 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Fri, 4 Sep 2026 09:57:45 -0400 Subject: [PATCH 1/4] fix(release): preflight the registry before publishing anything A new package cannot be released by CI until a human has published it once: npm's OIDC trusted publishing is configured per package on the registry, so there is nothing to configure until the package exists, and `npm trust` cannot create it (npm/cli#8544). CI holds no other npm credential by design, so its first `npm publish` fails with ENEEDAUTH. codec-libjxl landed in #88 and hit exactly that. Worse, the publish step was a bash loop under `set -e`, so it died where it stood -- and libjxl sits fifth in dependency order, so little-endian, openjpeg, openjph and dicom-codec were never attempted. Four packages that would have published fine sat stranded behind one that could not, for three days, each release leaving main tagged for versions that were not on npm. Resolve every package's registry state before publishing anything, so a release that cannot fully succeed publishes nothing and says what a human has to do. `npm view` reports a missing version and a missing package identically (E404), so the two lookups are separate; a non-zero exit that is NOT a 404 is now an error rather than being read as "brand new", which would turn a network blip into an aborted release. Fail-fast rather than skip-and-continue: publishing dicom-codec while a sibling whose range it carries has just failed is the window publish-order.mjs exists to close. The same check runs on every PR as a warning, which is what was missing when #88 merged -- on the PR that adds a codec, "not on npm yet" is simply true. Also: - Port setup-trusted-publishing.sh to node. It computed the repo root with `cd && pwd` and passed it as argv to node, so under Cygwin a Windows node.exe resolved /cygdrive/z/... against the current drive and the scan died with ENOENT. Nothing crosses a shell boundary now, and npm is spawned by its platform-correct name -- node refuses to spawn a .cmd without a shell since CVE-2024-27980, and passing an args array with shell:true is DEP0190, so npm.mjs handles both in one place. - Drive every release entry point from a root package.json script, so none of them depend on a shell. The publish job still installs no dependencies: `npm run` needs no node_modules, and these scripts import only node builtins. - Give packages/libjxl the repository.directory every sibling carries. - Document the bootstrap procedure in tools/release/README.md. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/pr-checks.yml | 18 +- .github/workflows/release.yml | 57 +++--- package.json | 6 +- packages/libjxl/package.json | 3 +- tools/release/README.md | 86 +++++++-- tools/release/npm.mjs | 92 ++++++++++ tools/release/publish-order.mjs | 42 +++-- tools/release/publish.mjs | 181 +++++++++++++++++++ tools/release/setup-trusted-publishing.mjs | 193 +++++++++++++++++++++ tools/release/setup-trusted-publishing.sh | 124 ------------- tools/release/version.mjs | 2 +- 11 files changed, 611 insertions(+), 193 deletions(-) create mode 100644 tools/release/npm.mjs create mode 100644 tools/release/publish.mjs create mode 100644 tools/release/setup-trusted-publishing.mjs delete mode 100644 tools/release/setup-trusted-publishing.sh diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 64b63b87..192a9adf 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -378,14 +378,20 @@ jobs: # Exercises the release scripts on every PR. They are otherwise only # ever executed by a live release — after the 8-job build matrix, and # partway through mutating manifests, which is a poor place to discover - # a syntax error or a bad tag walk. Mutates nothing (--dry-run), and - # publish-order.mjs additionally re-checks the topological order and - # that every package shipping dist/ actually has one. + # a syntax error or a bad tag walk. Mutates nothing and publishes + # nothing. + # + # release:preflight re-checks the topological order and that every + # package shipping dist/ actually has one, then asks the registry + # whether each package can actually be published. A package that is not + # on npm at all is only a WARNING here — the PR adding a codec is + # exactly when it legitimately does not exist yet — but it is the + # warning that was missing when libjxl was merged in #88 and broke the + # next four releases. In the release workflow the same finding is fatal. run: | set -euo pipefail - node tools/release/version.mjs --dry-run - echo "Publish order:" - node tools/release/publish-order.mjs + pnpm run release:plan + pnpm run release:preflight dist-size: # Binary-size regression gate: compares every shipped dist artifact diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7e09ca72..f1f57142 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -5,7 +5,7 @@ name: Release # THE FILENAME OF THIS WORKFLOW IS LOAD-BEARING. Each package's npm trusted # publisher is registered against `release.yml` in cornerstonejs/codecs, so npm # will reject an OIDC publish coming from any other workflow file. Renaming -# this file means re-running tools/release/setup-trusted-publishing.sh. +# this file means re-running `npm run release:trust`. # # This replaces the old CircleCI NPM_PUBLISH job, which needed two long-lived # personal credentials: an NPM_TOKEN written to ~/.npmrc, and a maintainer's @@ -509,24 +509,16 @@ jobs: cp -r "$d"/* "packages/$pkg/dist/" 2>/dev/null || true done ls packages/*/dist 2>/dev/null | head - - name: Compute publish order - # Generated as its own command on purpose. `done < <(cmd)` is process - # substitution, whose exit status `set -e` does not see: if - # publish-order.mjs refused an empty dist, the loop below would read - # nothing and the step would still go green, reporting "Published 0 - # packages" after the version commit and tags had already landed on - # main. Running it here makes that failure fail the job. - # - # publish-order.mjs emits the packages in dependency order, so - # dicom-codec goes out after the six siblings whose ranges it carries — - # publishing it alphabetically (third) left a window where those ranges - # resolved to versions that did not exist yet. It also refuses to emit a - # package that claims to ship dist/ but has an empty one. - run: | - set -euo pipefail - node tools/release/publish-order.mjs > publish-order.txt - cat publish-order.txt - name: Publish to npm + # One node script rather than a bash loop, for a reason the 2026-09-01 + # release paid for: `@cornerstonejs/codec-libjxl` was new, npm's OIDC + # trusted publishing cannot create a package that does not exist yet, so + # `npm publish` failed ENEEDAUTH — and `set -e` killed the loop where it + # stood, stranding the four already-registered packages queued behind it. + # publish.mjs resolves every package's registry state BEFORE it publishes + # anything, so a release that cannot fully succeed publishes nothing at + # all and says exactly what a human has to do. See its header. + # # Deliberately NOT gated on the plan. The source of truth is "every # workspace package whose current version is not yet on the registry", # which covers both the normal path (the versions version.mjs just @@ -534,30 +526,21 @@ jobs: # died partway through publishing, version.mjs correctly finds nothing # new to release on the re-run, and this step still finishes the job. # + # It also emits publish-order.txt for the github-releases job, in + # dependency order, so dicom-codec goes out after the six siblings whose + # ranges it carries — publishing it alphabetically (third) left a window + # where those ranges resolved to versions that did not exist yet — and + # refuses a package that claims to ship dist/ but has an empty one. + # # No .npmrc and no NODE_AUTH_TOKEN: `npm publish` performs the OIDC # exchange itself using the id-token permission above. Provenance is # generated automatically for trusted publishes, which is why every # package's repository.url must point at this repo. # - # --ignore-scripts: prepublishOnly re-runs `bash build.sh`, and this - # runner has no emscripten toolchain. The dist being published is the - # artifact the build job produced from this same commit. - run: | - set -euo pipefail - published=0 - skipped=0 - while read -r name version dir; do - if npm view "$name@$version" version >/dev/null 2>&1; then - echo "$name@$version is already on npm; skipping." - skipped=$((skipped + 1)) - continue - fi - - echo "Publishing $name@$version" - (cd "packages/$dir" && npm publish --ignore-scripts) - published=$((published + 1)) - done < publish-order.txt - echo "::notice::Published $published package(s); $skipped already on npm." + # `npm run` needs no node_modules — npm ships with node, and publish.mjs + # imports nothing but node builtins — so this job still installs no + # dependencies and the OIDC token still has no third-party code beside it. + run: npm run release:publish -- --out publish-order.txt - name: Upload publish order # Handed to the github-releases job so it can enumerate the same # packages without needing the dists (publish-order.mjs inspects them) diff --git a/package.json b/package.json index 039bd742..b77c6847 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,10 @@ "build:publish": "pnpm -r --parallel run build:ci", "csp:source": "node tools/csp/check-source-js.js", "docker:build": "bash tools/docker/build.sh", - "release:plan": "node tools/release/version.mjs --dry-run" + "release:plan": "node tools/release/version.mjs --dry-run", + "release:order": "node tools/release/publish-order.mjs", + "release:preflight": "node tools/release/publish.mjs --preflight", + "release:publish": "node tools/release/publish.mjs", + "release:trust": "node tools/release/setup-trusted-publishing.mjs" } } diff --git a/packages/libjxl/package.json b/packages/libjxl/package.json index 6012f264..8f3c71bd 100644 --- a/packages/libjxl/package.json +++ b/packages/libjxl/package.json @@ -43,6 +43,7 @@ ], "repository": { "type": "git", - "url": "git+https://github.com/cornerstonejs/codecs.git" + "url": "git+https://github.com/cornerstonejs/codecs.git", + "directory": "packages/libjxl" } } diff --git a/tools/release/README.md b/tools/release/README.md index 37e2ef55..405e9500 100644 --- a/tools/release/README.md +++ b/tools/release/README.md @@ -27,9 +27,21 @@ Preview what the next release would do, at any time, from a clean checkout with ```bash pnpm release:plan # node tools/release/version.mjs --dry-run +pnpm release:preflight # what would publish, and whether npm will accept it +pnpm release:order # just the dependency order node tools/release/version.mjs --dry-run --json # machine-readable ``` +Every release entry point is a root `package.json` script, so none of them depend on a shell: + +| Script | Does | +| --- | --- | +| `release:plan` | version bumps that the next release would make, mutating nothing | +| `release:order` | publishable packages in dependency order | +| `release:preflight` | the order, plus each package's registry state; publishes nothing | +| `release:publish` | the above, then publishes what is missing (used by the workflow) | +| `release:trust` | one-time trusted-publisher registration (run by a human) | + `--dry-run` writes nothing. Without it the script rewrites manifests and changelogs and emits `release-plan.json` (gitignored); the workflow is what commits, tags and pushes. @@ -47,7 +59,8 @@ third-party code that runs in it — the `actions/*` it calls and any dependency | `github-releases` | `contents: write` | **no** | `pnpm install` therefore never runs in a job that can publish to npm, and the job holding the OIDC -token runs nothing but `npm`, the pinned actions and `publish-order.mjs` (node builtins only). +token runs nothing but `npm`, the pinned actions and the scripts in this directory (node builtins +only — `npm run` needs no `node_modules`, since npm ships with node). 1. **`build`** — every package's `dist`, in the emscripten container (matrix job). Read-only, but its artifacts are what reaches npm, so its actions are SHA-pinned like the rest. @@ -64,9 +77,12 @@ token runs nothing but `npm`, the pinned actions and `publish-order.mjs` (node b run [32733067241](https://github.com/cornerstonejs/codecs/actions/runs/32733067241) left eight version tags on a commit that never reached `main` and wedged every later release at `git tag -a` with "tag already exists". -3. **`publish`** — checks out that SHA, replays the dists, and publishes each package with - `npm publish --ignore-scripts` in the dependency order `publish-order.mjs` computes — dicom-codec - goes out after the six siblings whose ranges it carries. `--ignore-scripts` is deliberate: +3. **`publish`** — checks out that SHA, replays the dists, and runs `npm run release:publish`, which + publishes each package with `npm publish --ignore-scripts` in the dependency order + `publish-order.mjs` computes — dicom-codec goes out after the six siblings whose ranges it + carries. Before publishing anything it resolves every package's registry state, so a release that + cannot fully succeed publishes nothing (see [Adding a new package](#adding-a-new-package)). + `--ignore-scripts` is deliberate: `prepublishOnly` re-runs `bash build.sh`, and this job has no emscripten toolchain — the dist being published is the artifact built in step 1 from the same commit. npm's version comes from the exactly-pinned `node-version` (v24.20.0 → npm 11.19.0, past the 11.5.1 OIDC floor), so there @@ -80,8 +96,9 @@ token runs nothing but `npm`, the pinned actions and `publish-order.mjs` (node b which is the only thing standing between a dropped build artifact and an empty package on npm for `libjpeg-turbo-12bit` (it has no vitest config, so the test gate never touches it). -Both scripts run on every PR as a dry-run step in `pr-checks.yml`, so they are not first executed -mid-release. +`release:plan` and `release:preflight` run on every PR in `pr-checks.yml`, so these scripts are not +first executed mid-release. Preflight is also where a newly added package gets flagged — as a +warning on the PR, and as a hard failure in the release. Every step is idempotent. If a run dies partway through publishing, re-run the workflow from the Actions tab (`workflow_dispatch`) and it finishes the job rather than double-publishing: `version.mjs` @@ -97,13 +114,21 @@ Both scripts are run by a human, once, and need credentials no CI job has. ### 1. npm trusted publishing ```bash -npm install --global npm@latest # needs >= 11.15.0 for `npm trust` -npm login # account with publish rights on @cornerstonejs, 2FA enabled -bash tools/release/setup-trusted-publishing.sh +npm login # account with publish rights on @cornerstonejs, 2FA enabled +pnpm release:trust ``` -This registers `cornerstonejs/codecs` + `release.yml` as the trusted publisher for all eight -packages. The first call prompts for a 2FA one-time password. +This registers `cornerstonejs/codecs` + `release.yml` as the trusted publisher for every package in +the workspace. The first call prompts for a 2FA one-time password. Re-running is safe: a package +that already has a config is reported and skipped. + +`npm trust` needs npm >= 11.15.0. The way to get it is **Node 24.20.0**, which bundles npm 11.19.0 — +the same version `release.yml` pins. Do not reach for `npm install --global npm@latest`: on an older +Node it refuses outright, because npm 12 requires `^22.22.2 || ^24.15.0 || >=26.0.0`. + +`npm trust` also needs a **web-login session**, not a token. A granular or classic token in +`~/.npmrc` publishes fine but fails here with `401 ... Bearer token authorization is required`, so +`npm login` is not optional even on a machine that can already publish. **The workflow's filename is part of the trust relationship.** Renaming `release.yml` breaks every publish until the script is re-run against the new name. @@ -116,6 +141,45 @@ After the first green release, harden on npmjs.com: set each package's *Publishi "Require two-factor authentication and disallow tokens", and delete the old `NPM_TOKEN` from the CircleCI project (CircleCI no longer runs anything for this repo — the project should be disabled). +## Adding a new package + +**A new package cannot be released by CI until a human has published it once.** npm's OIDC trusted +publishing is configured *per package, on the registry*, so there is nothing to configure until the +package exists — and `npm trust` cannot create it ([npm/cli#8544](https://github.com/npm/cli/issues/8544)). +CI has no other npm credential by design, so its first `npm publish` fails with `ENEEDAUTH`. + +This is not hypothetical. `@cornerstonejs/codec-libjxl` was merged in +[#88](https://github.com/cornerstonejs/codecs/pull/88) and every release for the next three days +failed on it, each one leaving `main` tagged for versions that were not on npm. Because the old +publish step was a bash loop under `set -e`, it died where it stood — and libjxl sits fifth in +dependency order, so `little-endian`, `openjpeg`, `openjph` and `dicom-codec` were never even +attempted. Four packages that would have published fine sat stranded behind one that could not. + +So, after merging a new codec: + +```bash +cd packages/ +npm publish --ignore-scripts # --ignore-scripts: prepublishOnly wants the emscripten toolchain +cd - +pnpm release:trust # registers release.yml for it; skips the rest +``` + +Then re-run the Release workflow. That one bootstrap version ships **without a provenance +attestation** — there was no trusted publisher to key it to — and every version after it has one. +Check with `npm view @ dist.attestations`. + +Two guards now make this loud instead of silent: + +- `release:preflight` runs on every PR and warns that the package is not on npm yet. It warns rather + than fails, because on the PR that adds the codec that is simply true. +- `release:publish` resolves every package's registry state *before* publishing anything, and aborts + the release with these instructions if one needs bootstrapping. Nothing is published, so no + release half-lands. + +Fail-fast is deliberate here, rather than skipping the bad package and continuing: publishing +`dicom-codec` while a sibling whose range it carries has just failed is precisely the window +`publish-order.mjs` exists to close. + ### 2. A push credential for `main` + the branch ruleset Two routes. **The deploy-key route needs only repo admin**; the App route is better hygiene but diff --git a/tools/release/npm.mjs b/tools/release/npm.mjs new file mode 100644 index 00000000..e2a79e4a --- /dev/null +++ b/tools/release/npm.mjs @@ -0,0 +1,92 @@ +// Spawning npm, correctly, on every platform the release scripts run on. +// +// Shared by publish.mjs and setup-trusted-publishing.mjs. Node builtins only — +// release.yml's publish job installs no dependencies on purpose (see its +// comments), so nothing in this directory may import from node_modules. +// +// Windows needs care twice over: +// +// 1. npm is a .cmd shim there, and since the fix for CVE-2024-27980 node +// refuses to spawn .bat/.cmd without a shell — it fails with EINVAL. A +// script that read that failure as "npm says no" would be badly wrong; +// the first draft of publish.mjs did exactly that and reported all nine +// published packages as brand-new. +// 2. Passing an args ARRAY together with `shell: true` is deprecated +// (DEP0190) because the args are concatenated without escaping. So build +// the one command string deliberately instead of letting node do it +// silently. Every argument the callers pass is a fixed flag or a registry +// spec — no spaces, no shell metacharacters, nothing from user input — so +// concatenation is safe here, and `assertShellSafe` keeps it that way if +// someone later passes something with a space in it. + +import { spawnSync } from 'node:child_process'; + +const NPM = process.platform === 'win32' ? 'npm.cmd' : 'npm'; +const NEEDS_SHELL = process.platform === 'win32'; + +function assertShellSafe(args) { + for (const arg of args) { + if (!/^[A-Za-z0-9@/._=-]+$/.test(arg)) { + throw new Error( + `refusing to pass ${JSON.stringify(arg)} to npm through a shell: ` + + 'it is not a plain flag or registry spec. Quote it properly, or spawn npm without a shell.' + ); + } + } +} + +/** + * Run npm and hand back the raw spawnSync result — status and stderr included, + * so callers can tell an expected non-zero exit (a 404 from `npm view`) from a + * genuine failure. Throws only when npm could not be started at all. + */ +export function runNpm(args, options = {}) { + const spawnOptions = { encoding: 'utf8', ...options }; + + const result = NEEDS_SHELL + ? (assertShellSafe(args), spawnSync([NPM, ...args].join(' '), { ...spawnOptions, shell: true })) + : spawnSync(NPM, args, spawnOptions); + + if (result.error) { + throw new Error(`could not run \`npm ${args.join(' ')}\`: ${result.error.message}`); + } + + return result; +} + +/** npm's own version, e.g. "11.19.0". */ +export function npmVersion() { + const result = runNpm(['--version'], { stdio: ['ignore', 'pipe', 'pipe'] }); + + if (result.status !== 0) { + throw new Error(`\`npm --version\` failed with exit ${result.status}`); + } + + return `${result.stdout}`.trim(); +} + +/** + * Whether a registry spec resolves, distinguishing "npm says it is not there" + * from "npm could not tell us". + * + * That distinction is a safety property, not pedantry: treating any non-zero + * exit as "not published" turns a network blip or an expired session into + * "this is a brand-new package", which is a conclusion the release acts on. + */ +export function resolvesOnRegistry(spec) { + const result = runNpm(['view', spec, 'version'], { stdio: ['ignore', 'pipe', 'pipe'] }); + + if (result.status === 0) { + return true; + } + + const stderr = `${result.stderr ?? ''}`; + if (/E404|404 Not Found|is not in this registry/.test(stderr)) { + return false; + } + + throw new Error( + `\`npm view ${spec} version\` failed with exit ${result.status}, and not with a 404. ` + + `Refusing to guess whether it is published.\n${stderr.trim()}` + ); +} diff --git a/tools/release/publish-order.mjs b/tools/release/publish-order.mjs index 539a3634..b623fe8c 100644 --- a/tools/release/publish-order.mjs +++ b/tools/release/publish-order.mjs @@ -23,11 +23,15 @@ // package after it. One stream of complete lines has no such edge. // // Usage: -// node tools/release/publish-order.mjs +// node tools/release/publish-order.mjs (or: npm run release:order) +// +// Also imported by publish.mjs, which needs the same order and the same dist +// check but drives the publishing itself. The CLI behaviour below runs only +// when this file is executed directly, so importing it has no side effects. import fs from 'node:fs'; import path from 'node:path'; -import { fileURLToPath } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); const PACKAGES_DIR = path.join(ROOT, 'packages'); @@ -116,21 +120,35 @@ function assertShippableContents(pkg) { } } -function main() { - const packages = readWorkspace(); - const ordered = topologicallySorted(packages); +/** + * The workspace's publishable packages, in dependency order, each verified to + * have the dist/ it claims to ship. Throws on a cycle or an empty dist. + */ +export function publishOrder() { + const ordered = topologicallySorted(readWorkspace()); for (const pkg of ordered) { assertShippableContents(pkg); } - process.stdout.write(ordered.map((p) => `${p.name} ${p.version} ${p.dir}`).join('\n')); - process.stdout.write('\n'); + return ordered; +} + +/** The ` ` lines the workflow's publish-order.txt holds. */ +export function formatOrder(ordered) { + return `${ordered.map((p) => `${p.name} ${p.version} ${p.dir}`).join('\n')}\n`; } -try { - main(); -} catch (error) { - console.error(`::error::${error.message}`); - process.exit(1); +function main() { + process.stdout.write(formatOrder(publishOrder())); +} + +// Executed directly, not imported. +if (process.argv[1] && pathToFileURL(process.argv[1]).href === import.meta.url) { + try { + main(); + } catch (error) { + console.error(`::error::${error.message}`); + process.exit(1); + } } diff --git a/tools/release/publish.mjs b/tools/release/publish.mjs new file mode 100644 index 00000000..54d3b052 --- /dev/null +++ b/tools/release/publish.mjs @@ -0,0 +1,181 @@ +#!/usr/bin/env node +// +// Publishes the workspace to npm, in dependency order, skipping versions that +// are already on the registry. +// +// This replaces the inline bash loop that used to live in release.yml's publish +// step. The reason it moved is the 2026-09-01 release: `@cornerstonejs/codec-libjxl` +// was a brand-new package, and npm's OIDC trusted publishing CANNOT create a +// package that does not exist yet -- a trusted publisher is configured per +// package, on the registry, so there is nothing to configure until the first +// version is there (npm/cli#8544). `npm publish` therefore failed with +// ENEEDAUTH, and because the loop ran under `set -euo pipefail` the step died on +// the spot -- stranding little-endian, openjpeg, openjph and dicom-codec, four +// packages that were already registered and would have published fine. Every +// release for the next three days failed the same way, each one leaving main +// tagged for versions that were not on npm. +// +// So the fix is not "keep going on error" -- publishing dicom-codec when a +// sibling whose range it carries has just failed is exactly the window +// publish-order.mjs exists to close. The fix is to ask the question BEFORE +// publishing anything: is every package we are about to publish actually +// publishable? A package that npm cannot accept is knowable up front, and a +// release that cannot fully succeed should decline to half-succeed. +// +// Usage: +// node tools/release/publish.mjs (npm run release:publish) +// node tools/release/publish.mjs --preflight (npm run release:preflight) +// node tools/release/publish.mjs --out FILE also write the publish order +// +// --preflight checks and reports without publishing, and treats a +// needs-bootstrap package as a WARNING rather than an error: on a pull request +// the package legitimately does not exist yet, and failing the PR that adds a +// codec would be wrong. Without it, needs-bootstrap is fatal and nothing is +// published. +// +// NODE BUILTINS ONLY, and it must stay that way. release.yml's publish job +// installs no dependencies on purpose, so that the only code running next to a +// credential which can publish to npm is npm itself and the dependency-free +// scripts in this directory. An import from node_modules here would quietly +// undo that boundary. + +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { publishOrder, formatOrder } from './publish-order.mjs'; +import { runNpm, resolvesOnRegistry } from './npm.mjs'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const PACKAGES_DIR = path.join(ROOT, 'packages'); + +/** + * What npm will let us do with this package, in one of three states: + * + * published this exact version is already on the registry -> skip it + * publishable the package exists, this version does not -> publish it + * needs-bootstrap the package NAME is not on the registry at all + * + * The third is the one that broke the release. `npm view` reports a missing + * version and a missing package the same way (E404), so the two lookups have + * to be separate to tell them apart. + */ +function registryState(pkg) { + if (resolvesOnRegistry(`${pkg.name}@${pkg.version}`)) { + return 'published'; + } + + return resolvesOnRegistry(pkg.name) ? 'publishable' : 'needs-bootstrap'; +} + +function bootstrapInstructions(packages) { + const lines = [ + `${packages.length} package(s) have never been published, and npm's OIDC trusted`, + 'publishing cannot create a package that does not exist yet -- the trusted publisher is', + 'configured per package on the registry, so the first version has to come from a human.', + 'Nothing was published; the release declined rather than publishing a partial set.', + '', + 'A maintainer with publish rights on @cornerstonejs and 2FA enabled should run, once:', + '', + ]; + + for (const pkg of packages) { + lines.push(` cd packages/${pkg.dir} && npm publish --ignore-scripts && cd -`); + } + + lines.push( + '', + ' npm run release:trust # registers release.yml as the trusted publisher', + '', + 'then re-run this workflow. See tools/release/README.md, "Adding a new package".' + ); + + return lines.join('\n'); +} + +function main() { + const args = process.argv.slice(2); + const preflightOnly = args.includes('--preflight'); + const outIndex = args.indexOf('--out'); + const outFile = outIndex === -1 ? null : args[outIndex + 1]; + + // Throws on a dependency cycle, or on a package that says it ships dist/ but + // has an empty one -- both before any network call or any publish. + const ordered = publishOrder(); + + if (outFile) { + fs.writeFileSync(outFile, formatOrder(ordered)); + } + + console.log(`Publish order (${ordered.length} packages):`); + for (const pkg of ordered) { + console.log(` ${pkg.name} ${pkg.version} (${pkg.dir})`); + } + console.log(''); + + // PREFLIGHT: resolve every package's state before touching the registry with + // a write. This is the whole point of the file -- see the header. + const state = new Map(); + for (const pkg of ordered) { + state.set(pkg.name, registryState(pkg)); + } + + const needsBootstrap = ordered.filter((p) => state.get(p.name) === 'needs-bootstrap'); + + if (needsBootstrap.length > 0) { + const message = bootstrapInstructions(needsBootstrap); + + if (!preflightOnly) { + console.error(`::error::${message}`); + process.exit(1); + } + + // Advisory on a PR: the package really does not exist yet, and the PR that + // adds a codec should not fail for it. It should say so loudly, though -- + // silence here is what let libjxl reach main and break four releases. + console.log(`::warning::${message}`); + } + + const toPublish = ordered.filter((p) => state.get(p.name) === 'publishable'); + const alreadyOn = ordered.filter((p) => state.get(p.name) === 'published'); + + for (const pkg of alreadyOn) { + console.log(`${pkg.name}@${pkg.version} is already on npm; skipping.`); + } + + if (preflightOnly) { + console.log(''); + console.log( + `Preflight: ${toPublish.length} to publish, ${alreadyOn.length} already on npm, ` + + `${needsBootstrap.length} awaiting a first manual publish.` + ); + return; + } + + // Fail-fast, deliberately. If a publish fails here it is not a case we + // predicted, and continuing would publish dicom-codec over ranges pointing at + // a sibling that did not make it. + for (const pkg of toPublish) { + console.log(`Publishing ${pkg.name}@${pkg.version}`); + // --ignore-scripts: prepublishOnly re-runs the emscripten build, and the + // publish job has no toolchain. The dist here is the build job's artifact. + const result = runNpm(['publish', '--ignore-scripts'], { + cwd: path.join(PACKAGES_DIR, pkg.dir), + stdio: 'inherit', + }); + + if (result.status !== 0) { + throw new Error(`npm publish failed for ${pkg.name}@${pkg.version} (exit ${result.status})`); + } + } + + console.log( + `::notice::Published ${toPublish.length} package(s); ${alreadyOn.length} already on npm.` + ); +} + +try { + main(); +} catch (error) { + console.error(`::error::${error.message}`); + process.exit(1); +} diff --git a/tools/release/setup-trusted-publishing.mjs b/tools/release/setup-trusted-publishing.mjs new file mode 100644 index 00000000..994ac0ab --- /dev/null +++ b/tools/release/setup-trusted-publishing.mjs @@ -0,0 +1,193 @@ +#!/usr/bin/env node +// +// One-time setup: register .github/workflows/release.yml as the npm trusted +// publisher for every package in this workspace. +// +// After this runs, the release workflow authenticates to npm with a short-lived +// OIDC token minted per run and scoped to that workflow -- no NPM_TOKEN, and a +// leaked token from anywhere else cannot publish these packages. +// +// Usage: +// npm run release:trust +// REPO=owner/name WORKFLOW=release.yml npm run release:trust +// +// PREREQUISITES +// 1. npm >= 11.15.0. The simplest way to get it is Node 24.20.0, which +// bundles npm 11.19.0 -- the same pin release.yml uses. `npm install +// --global npm@latest` will refuse on an older Node (npm 12 wants +// ^22.22.2 || ^24.15.0 || >=26.0.0). +// 2. Logged in interactively: npm login +// The account needs publish rights on @cornerstonejs and must have 2FA +// enabled. `npm trust` talks to an endpoint that only accepts a web-login +// SESSION token: a granular or classic access token sitting in ~/.npmrc +// fails with `401 ... Bearer token authorization is required`, even though +// the same token publishes fine. That is why this cannot run unattended. +// If you hit that 401, clear the stored token and log in again: +// npm logout # or delete the //registry.npmjs.org/:_authToken +// npm login # line from ~/.npmrc by hand if logout fails +// Note this replaces whatever token was in ~/.npmrc. +// +// Re-running is safe: npm allows exactly one publisher config per package, so a +// package that already has one is reported and skipped rather than duplicated. +// To replace an existing config, revoke it first: +// npm trust list +// npm trust revoke --id +// +// A package that is not yet ON the registry cannot be configured at all -- see +// "Adding a new package" in README.md. Such packages are reported and skipped +// here rather than failing the run, since the fix is a manual publish. +// +// WHY THIS IS NODE AND NOT BASH. It used to be setup-trusted-publishing.sh, +// which computed the repo root with `cd ... && pwd` and passed it as argv to +// node. Under Cygwin `pwd` yields /cygdrive/z/src/codecs, and a *Windows* +// node.exe resolves that leading slash against the current drive -- so the +// scan died with `ENOENT: scandir 'Z:\cygdrive\z\src\codecs\packages'`. There +// is no path handed across a shell boundary here, and npm is spawned via its +// platform-correct executable name, so this runs the same from cmd, +// PowerShell, Git Bash, Cygwin and CI. + +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { runNpm, npmVersion, resolvesOnRegistry } from './npm.mjs'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const PACKAGES_DIR = path.join(ROOT, 'packages'); + +const REPO = process.env.REPO || 'cornerstonejs/codecs'; +const WORKFLOW = process.env.WORKFLOW || 'release.yml'; +const REQUIRED_NPM = '11.15.0'; + +/** -1 / 0 / 1, on dotted numeric versions. Avoids a semver dependency. */ +function compareVersions(a, b) { + const pa = a.split('.').map(Number); + const pb = b.split('.').map(Number); + + for (let i = 0; i < Math.max(pa.length, pb.length); i += 1) { + const diff = (pa[i] ?? 0) - (pb[i] ?? 0); + if (diff !== 0) { + return Math.sign(diff); + } + } + + return 0; +} + +function requireNpmVersion() { + const current = npmVersion(); + + if (compareVersions(current, REQUIRED_NPM) < 0) { + console.error(`npm ${current} is too old for \`npm trust\` (need >= ${REQUIRED_NPM}).`); + console.error('Install Node 24.20.0, which bundles npm 11.19.0:'); + console.error(' https://nodejs.org/dist/v24.20.0/node-v24.20.0-x64.msi'); + process.exit(1); + } + + return current; +} + +/** + * Every publishable package in the workspace. + * + * Read from the workspace rather than hardcoded, so the list cannot drift. A + * package added or renamed without a trusted publisher of its own does not + * fail here -- it fails at `npm publish`, partway through a live release. + */ +function publishablePackages() { + const packages = []; + + for (const entry of fs.readdirSync(PACKAGES_DIR).sort()) { + const manifestPath = path.join(PACKAGES_DIR, entry, 'package.json'); + if (!fs.existsSync(manifestPath)) { + continue; + } + + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + if (manifest.private || !manifest.name) { + continue; + } + + packages.push(manifest.name); + } + + return packages; +} + +function main() { + const npmCurrent = requireNpmVersion(); + const packages = publishablePackages(); + + if (packages.length === 0) { + console.error(`No publishable packages found under ${PACKAGES_DIR}.`); + process.exit(1); + } + + console.log(`npm ${npmCurrent}`); + console.log( + `Registering ${REPO} / ${WORKFLOW} as trusted publisher for ${packages.length} packages.` + ); + console.log('The first package will prompt for your 2FA one-time password.'); + console.log(''); + + const failed = []; + const unpublished = []; + + for (const name of packages) { + console.log(`==> ${name}`); + + if (!resolvesOnRegistry(name)) { + console.log(' NOT ON NPM YET — skipping (needs a first manual publish; see README.md)'); + unpublished.push(name); + console.log(''); + continue; + } + + // stdio: inherit so npm's 2FA prompt reaches the terminal. + const result = runNpm( + ['trust', 'github', name, '--repo', REPO, '--file', WORKFLOW, '--allow-publish', '--yes'], + { stdio: 'inherit' } + ); + + if (result.status === 0) { + console.log(' ok'); + } else { + console.error(' FAILED — see the message above (an existing config must be revoked first)'); + failed.push(name); + } + + console.log(''); + } + + console.log('Current configuration:'); + for (const name of packages) { + console.log(`==> ${name}`); + // A package with no config exits non-zero; that is reported above, and is + // not worth failing this summary over. + runNpm(['trust', 'list', name], { stdio: 'inherit' }); + } + + if (unpublished.length > 0) { + console.log(''); + console.log(`${unpublished.length} package(s) are not on npm yet: ${unpublished.join(', ')}`); + console.log('Publish each once by hand, then re-run this script. See README.md.'); + } + + if (failed.length > 0) { + console.error(''); + console.error(`${failed.length} package(s) were not configured: ${failed.join(', ')}`); + process.exit(1); + } + + console.log(''); + console.log('Done. Next steps:'); + console.log(' - Merge the release workflow and watch the first run publish with provenance.'); + console.log(" - Then, on npmjs.com, set each package's Publishing access to"); + console.log(" 'Require two-factor authentication and disallow tokens'."); +} + +try { + main(); +} catch (error) { + console.error(error.message); + process.exit(1); +} diff --git a/tools/release/setup-trusted-publishing.sh b/tools/release/setup-trusted-publishing.sh deleted file mode 100644 index 15d3a085..00000000 --- a/tools/release/setup-trusted-publishing.sh +++ /dev/null @@ -1,124 +0,0 @@ -#!/usr/bin/env bash -# -# One-time setup: register .github/workflows/release.yml as the npm trusted -# publisher for every package in this workspace. -# -# After this runs, the release workflow authenticates to npm with a short-lived -# OIDC token minted per run and scoped to that workflow — no NPM_TOKEN, and a -# leaked token from anywhere else cannot publish these packages. -# -# PREREQUISITES -# 1. npm >= 11.15.0: npm install --global npm@latest -# 2. Logged in interactively: npm login -# The account needs publish rights on @cornerstonejs and must have 2FA -# enabled. `npm trust` talks to an endpoint that only accepts a web-login -# SESSION token: a granular or classic access token sitting in ~/.npmrc -# fails with `401 ... Bearer token authorization is required`, even though -# the same token publishes fine. That is why this cannot run unattended. -# If you hit that 401, clear the stored token and log in again: -# npm logout # or delete the //registry.npmjs.org/:_authToken -# npm login # line from ~/.npmrc by hand if logout fails -# Note this replaces whatever token was in ~/.npmrc. -# -# Re-running is safe: npm allows exactly one publisher config per package, so a -# package that already has one is reported and skipped rather than duplicated. -# To replace an existing config, revoke it first: -# npm trust list -# npm trust revoke --id - -set -euo pipefail - -REPO="${REPO:-cornerstonejs/codecs}" -WORKFLOW="${WORKFLOW:-release.yml}" - -ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" - -# Read from the workspace rather than hardcoded, so the list cannot drift. A -# package added or renamed without a trusted publisher of its own does not fail -# here — it fails at `npm publish`, partway through a live release, after the -# version commit and tags have already landed on main. -# -# node rather than jq: npm is a prerequisite of this script, so node is -# guaranteed present and jq is not. -# -# Via a temp file rather than `mapfile < <(node ...)`: process substitution's -# exit status is invisible to `set -e`, and the scanner prints as it walks, so a -# manifest that fails to parse halfway through would leave mapfile holding the -# packages emitted before the throw — and this script would go on to report -# success having configured only some of them. -SCAN_OUTPUT=$(mktemp) -trap 'rm -f "$SCAN_OUTPUT"' EXIT - -node -e ' - const fs = require("fs"); - const path = require("path"); - const dir = path.join(process.argv[1], "packages"); - - for (const entry of fs.readdirSync(dir).sort()) { - const manifest = path.join(dir, entry, "package.json"); - if (!fs.existsSync(manifest)) continue; - const pkg = JSON.parse(fs.readFileSync(manifest, "utf8")); - if (pkg.private || !pkg.name) continue; - console.log(pkg.name); - } -' "$ROOT" >"$SCAN_OUTPUT" - -mapfile -t PACKAGES <"$SCAN_OUTPUT" - -if [ ${#PACKAGES[@]} -eq 0 ]; then - echo "No publishable packages found under $ROOT/packages." >&2 - exit 1 -fi - -require_npm_version() { - local current required="11.15.0" - current=$(npm --version) - - # Sort the two versions and check the required one comes first. - if [ "$(printf '%s\n%s\n' "$required" "$current" | sort -V | head -n1)" != "$required" ]; then - echo "npm $current is too old for \`npm trust\` (need >= $required)." >&2 - echo "Run: npm install --global npm@latest" >&2 - exit 1 - fi -} - -require_npm_version - -echo "Registering $REPO / $WORKFLOW as trusted publisher for ${#PACKAGES[@]} packages." -echo "The first package will prompt for your 2FA one-time password." -echo - -failed=() -for pkg in "${PACKAGES[@]}"; do - echo "==> $pkg" - if npm trust github "$pkg" \ - --repo "$REPO" \ - --file "$WORKFLOW" \ - --allow-publish \ - --yes; then - echo " ok" - else - echo " FAILED — see the message above (an existing config must be revoked first)" >&2 - failed+=("$pkg") - fi - echo -done - -echo "Current configuration:" -for pkg in "${PACKAGES[@]}"; do - echo "==> $pkg" - npm trust list "$pkg" || true -done - -if [ ${#failed[@]} -gt 0 ]; then - echo >&2 - echo "${#failed[@]} package(s) were not configured: ${failed[*]}" >&2 - exit 1 -fi - -echo -echo "Done. Next steps:" -echo " - Merge the release workflow and watch the first run publish with provenance." -echo " - Then, on npmjs.com, set each package's Publishing access to" -echo " 'Require two-factor authentication and disallow tokens', and delete" -echo " the old NPM_TOKEN from the CircleCI project." diff --git a/tools/release/version.mjs b/tools/release/version.mjs index d07d9de0..2b7af1a7 100644 --- a/tools/release/version.mjs +++ b/tools/release/version.mjs @@ -118,7 +118,7 @@ const RECORD_SEP = String.fromCharCode(0x1e); // git emits it as %x1e // Carried over verbatim from lerna.json's command.publish.ignoreChanges. That // key looked like it applied only to `lerna publish`, but VersionCommand // declares publish as an "other command config" and read it too — so this is -// what stopped a docs-only commit from releasing all eight packages. A commit +// what stopped a docs-only commit from releasing every package. A commit // counts as releasable only if it touches at least one path these globs do // NOT match. const IGNORE_CHANGES = ['*.md', '*.yml', '*.spec.js', '*.test.js']; From 5100e9349260109f07fd2a7ae75608f29005f180 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Fri, 4 Sep 2026 14:51:24 -0400 Subject: [PATCH 2/4] fix(ci): stop the release commit cancelling main's bench baseline bench.yml groups by `bench-${{ github.head_ref || github.ref }}`, and on a push head_ref is empty -- so every push to main shared the group `bench-refs/heads/main`. With cancel-in-progress: true, the release workflow's version commit (pushed ~5 minutes after the merge that triggered it, into a bench that takes ~11) entered that group, cancelled the merge commit's bench, and was then skipped itself by the gate: 21:45 16f50e3 Expand `hrtime` utility... (#70) cancelled 21:50 91d91bc chore(release): publish skipped 16:56 21d4749 fix: consolidated codec fixes (#73) cancelled 17:01 7abaaa9 chore(release): publish skipped Those merges produced no baseline at all. The gate's guard exists to stop the version commit seeding a DUPLICATE baseline; paired with unconditional cancellation it destroyed the real one and put nothing in its place, so later PRs compared against whatever CodSpeed still held per benchmark. That is how this very PR -- which changes no runtime code -- drew a two-fold "regression" on two dicom-codec dispatch benches while charls reported a two-fold improvement against a pre-serialisation value. Cancel only for pull_request, which was the actual intent: PR churn should supersede itself, one main push must never cancel another. workflow_dispatch stops cancelling too, which is right -- that event is CodSpeed's backtest trigger. This was masked while releases were broken. A release that dies before the push cancels nothing, which is the only reason bac71dd kept its baseline. Fixing the publish path makes the version commit land reliably, so this would have started firing on most merges. Also document in BENCHMARKING.md the two things that CANNOT be fixed from the repo, since both are dashboard-only: archiving the 66 orphaned benchmark entries (harmless -- a skipped bench reuses its baseline on both sides, so its delta is always zero), and acknowledging a regression. Note that neither blocks a merge, because main's ruleset lists no required status checks at all. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/bench.yml | 31 +++++++++++++++++++++++++++---- BENCHMARKING.md | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 9add452d..9292018b 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -30,12 +30,35 @@ on: # the same commit (every main commit has one, from its push run). workflow_dispatch: -# Cancel in-flight benches when a new push lands on the same PR / branch. -# Killing a bench mid-run is safe for the shared-box mutex: flock releases -# with the process (see tools/ci/with-nashua-lock.sh). +# Cancel in-flight benches when a new push lands on the same PR. Killing a +# bench mid-run is safe for the shared-box mutex: flock releases with the +# process (see tools/ci/with-nashua-lock.sh). +# +# NEVER on a push to main, because on a push `head_ref` is empty and every main +# push therefore shares the group `bench-refs/heads/main`. With +# cancel-in-progress: true, the release workflow's own version commit — pushed +# about five minutes after the merge that triggered it, into a bench that takes +# eleven — entered that group, CANCELLED the merge commit's bench, and was then +# skipped itself by the gate below. The merge produced no baseline at all: +# +# 21:45 16f50e3 Expand `hrtime` utility... (#70) cancelled +# 21:50 91d91bc chore(release): publish skipped +# 16:56 21d4749 fix: consolidated codec fixes (#73) cancelled +# 17:01 7abaaa9 chore(release): publish skipped +# +# The gate's guard exists to stop the version commit seeding a DUPLICATE +# baseline; paired with unconditional cancellation it destroyed the real one and +# supplied nothing in its place. Every PR opened afterwards compared against +# whatever CodSpeed still held for each benchmark, which is how a diff touching +# no runtime code drew a two-fold "regression". +# +# This was masked while releases were broken: a release that dies before the +# push never cancels anything, so `bac71dd` kept its baseline by accident. Fixing +# the release makes the version commit land reliably, which would have made this +# fire on most merges from now on. concurrency: group: bench-${{ github.head_ref || github.ref }} - cancel-in-progress: true + cancel-in-progress: ${{ github.event_name == 'pull_request' }} permissions: contents: read diff --git a/BENCHMARKING.md b/BENCHMARKING.md index 94792eb5..97bab4ac 100644 --- a/BENCHMARKING.md +++ b/BENCHMARKING.md @@ -144,6 +144,38 @@ decode is the time spent" for wasm code — that's a fundamental limitation of wasm-via-V8-via-Cachegrind. For wasm hotspot analysis, profile the native build with `perf` / Instruments / VTune instead. +### "N benchmarks were skipped, so the baseline results were used instead" +The CodSpeed project holds more benchmark entries than the repo now +defines — as of 2026-09-04, 133 registered against 67 that actually run. +The surplus are orphans left by benches that were renamed or deleted +(the cold/warm split, the openjph upstream rework), plus +`libjpeg-turbo-12bit`, whose `bench` script is deliberately a no-op +(`.51` transfer syntax is disabled) so its 9th bench file never executes. + +**What to do**: nothing, for correctness. A skipped benchmark reuses its +baseline on *both* sides of the comparison, so its delta is zero and it +can never trigger a regression — the only cost is a misleading total. +Archiving them is **dashboard-only** (Settings → the benchmark → Archive); +there is no repo config, config file or code annotation that clears them, +so this cannot be fixed in a commit. + +### A regression on a diff that changes no runtime code +Almost always a stale baseline rather than a real change. The usual cause +was a main baseline that never got measured: until 2026-09-04 the release +workflow's version commit shared `bench.yml`'s push concurrency group with +the merge commit it followed, so it cancelled that bench and then skipped +its own — see the comment on `concurrency:` in +[.github/workflows/bench.yml](.github/workflows/bench.yml). Merges of #70 +and #73 produced no baseline at all. + +**What to do**: confirm the diff cannot affect the measured path, then let +the next successful **push to main** re-seed the baseline for all live +benchmarks — that happens on its own and needs no dashboard access. +Acknowledging a regression directly *is* dashboard-only and admin-only, but +it is not required to merge: main's ruleset lists no required status checks +(only 1 approving review and code-owner review), so a red CodSpeed check +never blocks a pull request. + ### Tier-related regression in `instantiate+destroy X` The instantiation bench measures whatever V8 tier the embind helpers (`makeClassHandle`, `RegisteredPointer_fromWireType`, etc.) happen to From 4d43aa244faba2d42cf484d303cdd78f38fe7980 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Tue, 8 Sep 2026 12:31:34 -0400 Subject: [PATCH 3/4] fix(ci,release): address review findings on bench concurrency and trust setup bench.yml: key non-PR runs by commit, not branch. cancel-in-progress: false stops a new run killing a RUNNING one, but a concurrency group still holds only one PENDING run and queueing a third cancels it. With an eleven minute bench, two merges inside that window put the second in the pending slot where a third could evict it - losing a main baseline exactly as before, by another route. github.sha gives every main commit its own group; the bench box's mutex still serialises them, queueing rather than discarding. PRs keep the branch group, where superseding an in-flight bench is the point. setup-trusted-publishing.mjs: stop claiming trusted publishing neutralises a leaked token. `npm trust github` adds an authorized path and revokes nothing - every token that could publish before still can, until each package's Publishing access is set to "Require two-factor authentication and disallow tokens" by hand. The script prints that as a next step and cannot verify it, so the header now says so rather than implying the opposite. setup-trusted-publishing.mjs: skip packages already trusting REPO/WORKFLOW. npm permits one publisher config per package and `npm trust github` fails rather than updating, including when the existing config is identical, so a fully configured workspace recorded nine failures and exited 1 on every re-run - contradicting the "re-running is safe" note directly above. Configs are now read first; ours is skipped as done, one pointing elsewhere is still a failure. The reader tolerates unknown field shapes and biases to attempting the create, because wrongly skipping leaves a package unconfigured until a live release trips over it. BENCHMARKING.md: subject-verb agreement. Co-Authored-By: Claude Opus 5 --- .github/workflows/bench.yml | 16 +++- BENCHMARKING.md | 2 +- tools/release/setup-trusted-publishing.mjs | 98 ++++++++++++++++++++-- 3 files changed, 108 insertions(+), 8 deletions(-) diff --git a/.github/workflows/bench.yml b/.github/workflows/bench.yml index 9292018b..04fc7075 100644 --- a/.github/workflows/bench.yml +++ b/.github/workflows/bench.yml @@ -56,8 +56,22 @@ on: # push never cancels anything, so `bac71dd` kept its baseline by accident. Fixing # the release makes the version commit land reliably, which would have made this # fire on most merges from now on. +# +# cancel-in-progress: false is NOT sufficient on its own, which is why the group +# is per-commit off a PR rather than per-branch. A concurrency group holds one +# running run and at most one PENDING run; queueing a third CANCELS the pending +# one, whatever cancel-in-progress says. A bench takes eleven minutes, so two +# merges inside that window would have put the second in the pending slot and +# let a third evict it -- the same lost baseline this is meant to fix, reached +# by a different route. +# +# Keying non-PR runs by github.sha gives every main commit its own group, so no +# main bench can ever cancel another. They serialise anyway: the bench box takes +# a mutex (tools/ci/with-nashua-lock.sh), which queues them without discarding +# any. PRs keep the branch-level group, where superseding an in-flight bench with +# a newer push is exactly what is wanted. concurrency: - group: bench-${{ github.head_ref || github.ref }} + group: bench-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} permissions: diff --git a/BENCHMARKING.md b/BENCHMARKING.md index 97bab4ac..6753dd6f 100644 --- a/BENCHMARKING.md +++ b/BENCHMARKING.md @@ -147,7 +147,7 @@ profile the native build with `perf` / Instruments / VTune instead. ### "N benchmarks were skipped, so the baseline results were used instead" The CodSpeed project holds more benchmark entries than the repo now defines — as of 2026-09-04, 133 registered against 67 that actually run. -The surplus are orphans left by benches that were renamed or deleted +The surplus entries are orphans left by benches that were renamed or deleted (the cold/warm split, the openjph upstream rework), plus `libjpeg-turbo-12bit`, whose `bench` script is deliberately a no-op (`.51` transfer syntax is disabled) so its 9th bench file never executes. diff --git a/tools/release/setup-trusted-publishing.mjs b/tools/release/setup-trusted-publishing.mjs index 994ac0ab..35190512 100644 --- a/tools/release/setup-trusted-publishing.mjs +++ b/tools/release/setup-trusted-publishing.mjs @@ -4,8 +4,17 @@ // publisher for every package in this workspace. // // After this runs, the release workflow authenticates to npm with a short-lived -// OIDC token minted per run and scoped to that workflow -- no NPM_TOKEN, and a -// leaked token from anywhere else cannot publish these packages. +// OIDC token minted per run and scoped to that workflow, so the release itself +// needs no NPM_TOKEN. +// +// It does NOT make a leaked token harmless. `npm trust github` ADDS an +// authorized publishing path; it revokes nothing. Every access token that could +// publish these packages before can still publish them afterwards. Token +// publishing stops only when each package's Publishing access on npmjs.com is +// set to "Require two-factor authentication and disallow tokens" -- a manual, +// per-package step that this script cannot perform and does not verify. It is +// printed as a next step at the end of a successful run; until it is done for +// every package, treat the old tokens as live credentials. // // Usage: // npm run release:trust @@ -27,9 +36,17 @@ // npm login # line from ~/.npmrc by hand if logout fails // Note this replaces whatever token was in ~/.npmrc. // -// Re-running is safe: npm allows exactly one publisher config per package, so a -// package that already has one is reported and skipped rather than duplicated. -// To replace an existing config, revoke it first: +// Re-running is safe. npm allows exactly one publisher config per package, and +// `npm trust github` fails rather than updating an existing one -- including +// when the existing config is byte-for-byte what we would have created. So each +// package's current config is read first, and one that already points at +// REPO/WORKFLOW is skipped as done rather than retried and counted as a +// failure. Without that check a fully configured workspace exited 1 on every +// re-run. +// +// A config that exists but points somewhere ELSE is still a failure: that is a +// real conflict, and resolving it means deciding which one is right. Revoke it +// first if the answer is ours: // npm trust list // npm trust revoke --id // @@ -113,6 +130,59 @@ function publishablePackages() { return packages; } +/** + * Whether `name` already trusts REPO/WORKFLOW. + * + * Returns 'match' (ours, nothing to do), 'other' (a config exists but is not + * ours) or 'unknown'. 'unknown' covers no config, no permission to read one, + * and output this cannot parse. + * + * Deliberately biased towards 'unknown': the caller attempts the create on + * anything that is not a confident 'match', so a shape this does not recognise + * costs a redundant `npm trust github` and the error it already handled. The + * opposite bias would silently skip a package that has no trusted publisher at + * all, and that is not discovered until a release tries to publish it. + * + * `npm trust list --json` is not covered by npm's documented output contract, + * hence the tolerance about field names. + */ +function existingTrust(name) { + const result = runNpm(['trust', 'list', name, '--json'], { stdio: ['ignore', 'pipe', 'pipe'] }); + + if (result.status !== 0) { + return 'unknown'; + } + + let parsed; + try { + parsed = JSON.parse(`${result.stdout}`); + } catch { + return 'unknown'; + } + + // Accept a bare array or any single-key wrapper around one. + const entries = Array.isArray(parsed) + ? parsed + : Object.values(parsed ?? {}).find(Array.isArray) ?? []; + + if (entries.length === 0) { + return 'unknown'; + } + + const matches = entries.some((entry) => { + if (!entry || typeof entry !== 'object') { + return false; + } + + const repo = entry.repository ?? entry.repo ?? entry.project; + const file = entry.file ?? entry.workflow ?? entry.workflowFilename; + + return repo === REPO && file === WORKFLOW; + }); + + return matches ? 'match' : 'other'; +} + function main() { const npmCurrent = requireNpmVersion(); const packages = publishablePackages(); @@ -131,6 +201,7 @@ function main() { const failed = []; const unpublished = []; + const alreadyConfigured = []; for (const name of packages) { console.log(`==> ${name}`); @@ -142,6 +213,13 @@ function main() { continue; } + if (existingTrust(name) === 'match') { + console.log(` already trusts ${REPO} / ${WORKFLOW} — skipping`); + alreadyConfigured.push(name); + console.log(''); + continue; + } + // stdio: inherit so npm's 2FA prompt reaches the terminal. const result = runNpm( ['trust', 'github', name, '--repo', REPO, '--file', WORKFLOW, '--allow-publish', '--yes'], @@ -151,7 +229,7 @@ function main() { if (result.status === 0) { console.log(' ok'); } else { - console.error(' FAILED — see the message above (an existing config must be revoked first)'); + console.error(' FAILED — see the message above (a conflicting config must be revoked first)'); failed.push(name); } @@ -166,6 +244,14 @@ function main() { runNpm(['trust', 'list', name], { stdio: 'inherit' }); } + if (alreadyConfigured.length > 0) { + console.log(''); + console.log( + `${alreadyConfigured.length} package(s) already trusted ${REPO} / ${WORKFLOW}: ` + + `${alreadyConfigured.join(', ')}` + ); + } + if (unpublished.length > 0) { console.log(''); console.log(`${unpublished.length} package(s) are not on npm yet: ${unpublished.join(', ')}`); From c6dcf80d357d9cfabd81fa8f29dac7016e17ace9 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Tue, 8 Sep 2026 13:59:24 -0400 Subject: [PATCH 4/4] fix(release): keep the bootstrap annotation to one line GitHub renders only the first line of a ::error:: or ::warning:: message in the Checks tab, so emitting the multi-line bootstrap instructions as the annotation showed them cut off mid-sentence -- "1 package(s) have never been published, and npm's OIDC trusted" and no more. The rest was still in the raw log, so nothing was lost, but the part a maintainer sees without opening the job was a fragment. Split the two: bootstrapSummary() is one self-contained line naming the packages, and the existing instructions follow as ordinary log lines. Each annotation and its instructions go to the same stream so they stay adjacent. Chose this over encoding the newlines as %0A because the annotation box is better as a summary than as a twelve-line block, and because naming the packages is the part worth having in the Checks tab. Both paths exercised locally against a throwaway package name npm has never seen: --preflight emits the one-line ::warning:: followed by the full instructions, and the release path emits the one-line ::error::, the instructions, and exits 1 without reaching the publish loop. Reported by jbocce in review of #93. Co-Authored-By: Claude Opus 5 --- tools/release/publish.mjs | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/tools/release/publish.mjs b/tools/release/publish.mjs index 54d3b052..60f9b3a4 100644 --- a/tools/release/publish.mjs +++ b/tools/release/publish.mjs @@ -67,6 +67,27 @@ function registryState(pkg) { return resolvesOnRegistry(pkg.name) ? 'publishable' : 'needs-bootstrap'; } +/** + * The one-line form, for a `::error::`/`::warning::` annotation. + * + * GitHub renders only the FIRST line of an annotation's message in the Checks + * tab, so emitting the multi-line instructions there showed them cut off + * mid-sentence ("...and npm's OIDC trusted"). The annotation therefore has to + * say the whole thing in one line and point at the log for the commands, which + * is why this is separate from bootstrapInstructions rather than its first + * line. Naming the packages here matters: the annotation is the part a + * maintainer sees without opening the job. + */ +function bootstrapSummary(packages) { + const names = packages.map((p) => p.name).join(', '); + + return ( + `${packages.length} package(s) have never been published and need one manual ` + + `publish before trusted publishing can release them: ${names}. ` + + 'Nothing was published; the job log has the exact commands.' + ); +} + function bootstrapInstructions(packages) { const lines = [ `${packages.length} package(s) have never been published, and npm's OIDC trusted`, @@ -122,17 +143,23 @@ function main() { const needsBootstrap = ordered.filter((p) => state.get(p.name) === 'needs-bootstrap'); if (needsBootstrap.length > 0) { - const message = bootstrapInstructions(needsBootstrap); + // One line for the annotation, the instructions as ordinary log lines -- + // see bootstrapSummary for why they cannot be the same string. Each pair + // goes to a single stream so the two stay adjacent in the log. + const summary = bootstrapSummary(needsBootstrap); + const instructions = bootstrapInstructions(needsBootstrap); if (!preflightOnly) { - console.error(`::error::${message}`); + console.error(`::error::${summary}`); + console.error(instructions); process.exit(1); } // Advisory on a PR: the package really does not exist yet, and the PR that // adds a codec should not fail for it. It should say so loudly, though -- // silence here is what let libjxl reach main and break four releases. - console.log(`::warning::${message}`); + console.log(`::warning::${summary}`); + console.log(instructions); } const toPublish = ordered.filter((p) => state.get(p.name) === 'publishable');