From be6eac163991c8181d4b26c45e0c368ef4842eee Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Wed, 5 Aug 2026 17:52:37 +0200 Subject: [PATCH 1/3] fix(release): bump package-lock.json alongside package.json prerelease.yml and promote.yml rewrote the version with a sed over package.json alone, so every release shipped a lockfile whose root version disagreed with the package it locks: v1.7.0 package.json=1.7.0 lock=1.6.0 v1.8.0 package.json=1.8.0 lock=1.8.0-rc.4 v1.9.0 package.json=1.9.0 lock=1.8.0 It went unnoticed for three releases because npm ci only fails on dependency drift, never on this field. The mismatch is inert until someone reads the diff, which is how it finally surfaced. Both workflows now call one script that writes package.json and both root version fields of the lockfile (lockfileVersion 3 repeats it under packages[""]). A plain sed cannot do this: the lockfile has a "version" key per dependency, so a naive substitution would rewrite the whole tree. The files are tab-indented JSON that JSON.stringify round-trips byte for byte, so rewriting them whole still yields a three-line diff. That is load-bearing rather than incidental, and the test pins it: if npm ever changes its lockfile formatting, the test fails instead of a release commit silently becoming a 40k-line reformat. The script refuses to write when packages[""] is absent rather than skipping it through optional chaining, since a silent half-bump is the exact failure being fixed. --- .github/scripts/set-release-version.mjs | 66 +++++++++++ .github/scripts/set-release-version.test.mjs | 115 +++++++++++++++++++ .github/workflows/prerelease.yml | 8 +- .github/workflows/promote.yml | 12 +- 4 files changed, 191 insertions(+), 10 deletions(-) create mode 100644 .github/scripts/set-release-version.mjs create mode 100644 .github/scripts/set-release-version.test.mjs diff --git a/.github/scripts/set-release-version.mjs b/.github/scripts/set-release-version.mjs new file mode 100644 index 000000000..0559e611b --- /dev/null +++ b/.github/scripts/set-release-version.mjs @@ -0,0 +1,66 @@ +#!/usr/bin/env node +// Sets the release version in package.json AND package-lock.json. +// +// prerelease.yml and promote.yml used to `sed` package.json alone, so every +// release shipped a lockfile whose root version disagreed with the package it +// locks: +// +// v1.7.0 package.json=1.7.0 lock=1.6.0 +// v1.8.0 package.json=1.8.0 lock=1.8.0-rc.4 +// v1.9.0 package.json=1.9.0 lock=1.8.0 +// +// Nothing caught it for three releases because `npm ci` only fails on +// dependency drift, never on this field — the mismatch is inert until someone +// reads the diff, which is how it was eventually noticed. +// +// Both files are tab-indented JSON that JSON.stringify round-trips byte for +// byte, so rewriting them whole still produces a one-line-per-file diff. The +// test pins that: if npm ever changes how it formats a lockfile, a release +// commit would otherwise silently become a 40k-line reformat. + +import { readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { argv } from "node:process"; + +/** + * @param {string} version Version to write, e.g. "1.9.0" or "1.9.0-rc.2". + * @param {string} dir Directory holding package.json and package-lock.json. + */ +export function setReleaseVersion(version, dir) { + if (!version) throw new Error("a version is required"); + + const edit = (name, mutate) => { + const file = join(dir, name); + const json = JSON.parse(readFileSync(file, "utf8")); + mutate(json); + writeFileSync(file, `${JSON.stringify(json, null, "\t")}\n`); + }; + + edit("package.json", (pkg) => { + pkg.version = version; + }); + + edit("package-lock.json", (lock) => { + lock.version = version; + // lockfileVersion 3 repeats the root version inside packages[""]. Optional + // chaining would quietly skip it if the shape ever changed — the same + // silent half-bump this script exists to end — so demand it instead. + if (!lock.packages?.[""]) { + throw new Error( + 'package-lock.json has no packages[""] entry; the lockfile format changed and this script needs updating', + ); + } + lock.packages[""].version = version; + }); +} + +// Only run when invoked directly, so the test can import the function. +if (import.meta.filename === argv[1]) { + const version = argv[2]; + if (!version) { + console.error("usage: node .github/scripts/set-release-version.mjs "); + process.exit(1); + } + setReleaseVersion(version, process.cwd()); + console.log(`version ${version} set in package.json and package-lock.json`); +} diff --git a/.github/scripts/set-release-version.test.mjs b/.github/scripts/set-release-version.test.mjs new file mode 100644 index 000000000..baf80a8cb --- /dev/null +++ b/.github/scripts/set-release-version.test.mjs @@ -0,0 +1,115 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { setReleaseVersion } from "./set-release-version.mjs"; + +let dir; + +// Tab-indented, like the real files, and shaped like lockfileVersion 3 — the +// point of most of these assertions is formatting, so the fixtures have to be +// byte-faithful rather than merely structurally right. +const pkg = [ + "{", + '\t"name": "openscreen",', + '\t"version": "1.8.0",', + '\t"private": true', + "}", + "", +].join("\n"); + +const lock = [ + "{", + '\t"name": "openscreen",', + '\t"version": "1.8.0",', + '\t"lockfileVersion": 3,', + '\t"requires": true,', + '\t"packages": {', + '\t\t"": {', + '\t\t\t"name": "openscreen",', + '\t\t\t"version": "1.8.0",', + '\t\t\t"dependencies": {', + '\t\t\t\t"zod": "^4.0.0"', + "\t\t\t}", + "\t\t},", + '\t\t"node_modules/zod": {', + '\t\t\t"version": "4.0.0"', + "\t\t}", + "\t}", + "}", + "", +].join("\n"); + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "set-release-version-")); + writeFileSync(join(dir, "package.json"), pkg); + writeFileSync(join(dir, "package-lock.json"), lock); +}); + +afterEach(() => { + rmSync(dir, { recursive: true, force: true }); +}); + +const read = (name) => readFileSync(join(dir, name), "utf8"); + +describe("setReleaseVersion", () => { + it("sets the version in package.json and both lockfile roots", () => { + setReleaseVersion("1.9.0", dir); + + expect(JSON.parse(read("package.json")).version).toBe("1.9.0"); + const written = JSON.parse(read("package-lock.json")); + expect(written.version).toBe("1.9.0"); + expect(written.packages[""].version).toBe("1.9.0"); + }); + + it("accepts a prerelease version", () => { + setReleaseVersion("2.0.0-rc.3", dir); + + expect(JSON.parse(read("package.json")).version).toBe("2.0.0-rc.3"); + expect(JSON.parse(read("package-lock.json")).packages[""].version).toBe("2.0.0-rc.3"); + }); + + // The reason the script may rewrite these files wholesale: anything else in + // them must come back out byte for byte. If npm changes its lockfile + // formatting, this fails here rather than turning a release commit into a + // 40k-line reformat nobody reviews. + it("changes only the version lines, leaving formatting untouched", () => { + setReleaseVersion("1.9.0", dir); + + const diff = (before, after) => { + const a = before.split("\n"); + const b = after.split("\n"); + expect(b.length).toBe(a.length); + return a.map((line, i) => [line, b[i]]).filter(([x, y]) => x !== y); + }; + + expect(diff(pkg, read("package.json"))).toEqual([ + ['\t"version": "1.8.0",', '\t"version": "1.9.0",'], + ]); + expect(diff(lock, read("package-lock.json"))).toEqual([ + ['\t"version": "1.8.0",', '\t"version": "1.9.0",'], + ['\t\t\t"version": "1.8.0",', '\t\t\t"version": "1.9.0",'], + ]); + }); + + it("leaves dependency versions alone", () => { + setReleaseVersion("1.9.0", dir); + + const written = JSON.parse(read("package-lock.json")); + expect(written.packages["node_modules/zod"].version).toBe("4.0.0"); + }); + + // A lockfile format change must stop the release, not half-bump it. + it("throws rather than half-bumping when the lockfile shape is unknown", () => { + writeFileSync( + join(dir, "package-lock.json"), + `${JSON.stringify({ name: "openscreen", version: "1.8.0" }, null, "\t")}\n`, + ); + + expect(() => setReleaseVersion("1.9.0", dir)).toThrow(/packages/); + }); + + it("requires a version", () => { + expect(() => setReleaseVersion("", dir)).toThrow(/version is required/); + }); +}); diff --git a/.github/workflows/prerelease.yml b/.github/workflows/prerelease.yml index 8723005fe..5a3e1c56d 100644 --- a/.github/workflows/prerelease.yml +++ b/.github/workflows/prerelease.yml @@ -106,10 +106,10 @@ jobs: echo "Creating release branch ${BRANCH} from ${GITHUB_REF_NAME}" git checkout -b "$BRANCH" fi - sed -i -E "s|(\"version\"[[:space:]]*:[[:space:]]*\")[^\"]*(\")|\1${PRERELEASE}\2|" package.json - echo "package.json version:" - grep '"version"' package.json - git add package.json + # Writes package-lock.json too. A sed over package.json alone left the + # lockfile behind on every release up to 1.9.0; see the script header. + node .github/scripts/set-release-version.mjs "${PRERELEASE}" + git add package.json package-lock.json git commit -m "chore(release): bump to ${PRERELEASE} [skip ci]" || echo "(version already at ${PRERELEASE})" git push "$REMOTE" "$BRANCH" diff --git a/.github/workflows/promote.yml b/.github/workflows/promote.yml index 7e76a6c79..0e01f4b8e 100644 --- a/.github/workflows/promote.yml +++ b/.github/workflows/promote.yml @@ -59,25 +59,25 @@ jobs: STABLE_VERSION: ${{ steps.version.outputs.stable_version }} run: node .github/scripts/release-milestone-close.mjs - - name: Bump package.json to stable version on the release branch + - name: Bump the version to stable on the release branch env: TOKEN: ${{ secrets.OPENSCREEN_RELEASE_TOKEN }} STABLE_VERSION: ${{ steps.version.outputs.stable_version }} run: | set -euo pipefail # Promote checks out the FROZEN release branch (created by prerelease.yml) and - # rewrites package.json there. This guarantees the stable tag points at the + # rewrites the version there. This guarantees the stable tag points at the # same code that was tested as the RC plus any cherry-picked bugfixes. BRANCH="release/v${STABLE_VERSION}" git fetch origin "$BRANCH" git checkout "$BRANCH" git reset --hard "origin/${BRANCH}" - sed -i -E "s|(\"version\"[[:space:]]*:[[:space:]]*\")[^\"]*(\")|\1${STABLE_VERSION}\2|" package.json - echo "package.json version:" - grep '"version"' package.json + # Writes package-lock.json too. A sed over package.json alone left the + # lockfile behind on every release up to 1.9.0; see the script header. + node .github/scripts/set-release-version.mjs "${STABLE_VERSION}" git config user.name "github-actions[bot]" git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git add package.json + git add package.json package-lock.json git commit --allow-empty -m "chore(release): bump to ${STABLE_VERSION} [skip ci]" || true git push "https://x-access-token:${TOKEN}@github.com/${GITHUB_REPOSITORY}.git" "$BRANCH" From 1b4f35bff989077d07851b03ea7895495ba27917 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Sat, 8 Aug 2026 23:17:13 +0200 Subject: [PATCH 2/3] fix(release): validate both manifests before writing either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The script wrote package.json, then validated the lockfile. A lockfile without packages[""] therefore left package.json bumped and the lockfile untouched — the exact half-bump this script exists to end, reproduced by its own error path. Both files are now read and validated up front and written only once every check has passed. The existing test claimed to cover this ("throws rather than half-bumping") but only asserted the throw; it passes unchanged against the buggy script. It now asserts package.json is still at 1.8.0 and the lockfile never saw 1.9.0, and fails against the previous implementation with expected '1.9.0' to be '1.8.0'. Also pins the direct-invocation guard with a CLI test. CodeRabbit read `import.meta.filename === argv[1]` as always false because the workflows pass a relative path; Node resolves argv[1] before exposing it, so it holds. Nothing covered that, and the whole script is dead code if it ever stops being true. --- .github/scripts/set-release-version.mjs | 44 +++++++++++--------- .github/scripts/set-release-version.test.mjs | 28 ++++++++++++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/.github/scripts/set-release-version.mjs b/.github/scripts/set-release-version.mjs index 0559e611b..24f6d2315 100644 --- a/.github/scripts/set-release-version.mjs +++ b/.github/scripts/set-release-version.mjs @@ -29,29 +29,35 @@ import { argv } from "node:process"; export function setReleaseVersion(version, dir) { if (!version) throw new Error("a version is required"); - const edit = (name, mutate) => { + // Both manifests are read and validated before a single byte is written. The + // obvious order — write package.json, then validate the lockfile — left + // package.json bumped and the lockfile untouched whenever validation failed, + // which is precisely the half-bump this script exists to end. Its own error + // path must not reproduce the bug it fixes. + const load = (name) => { const file = join(dir, name); - const json = JSON.parse(readFileSync(file, "utf8")); - mutate(json); - writeFileSync(file, `${JSON.stringify(json, null, "\t")}\n`); + return { file, json: JSON.parse(readFileSync(file, "utf8")) }; }; - edit("package.json", (pkg) => { - pkg.version = version; - }); + const pkg = load("package.json"); + const lock = load("package-lock.json"); + + // lockfileVersion 3 repeats the root version inside packages[""]. Optional + // chaining would quietly skip it if the shape ever changed — the same + // silent half-bump this script exists to end — so demand it instead. + if (!lock.json.packages?.[""]) { + throw new Error( + 'package-lock.json has no packages[""] entry; the lockfile format changed and this script needs updating', + ); + } + + pkg.json.version = version; + lock.json.version = version; + lock.json.packages[""].version = version; - edit("package-lock.json", (lock) => { - lock.version = version; - // lockfileVersion 3 repeats the root version inside packages[""]. Optional - // chaining would quietly skip it if the shape ever changed — the same - // silent half-bump this script exists to end — so demand it instead. - if (!lock.packages?.[""]) { - throw new Error( - 'package-lock.json has no packages[""] entry; the lockfile format changed and this script needs updating', - ); - } - lock.packages[""].version = version; - }); + for (const { file, json } of [pkg, lock]) { + writeFileSync(file, `${JSON.stringify(json, null, "\t")}\n`); + } } // Only run when invoked directly, so the test can import the function. diff --git a/.github/scripts/set-release-version.test.mjs b/.github/scripts/set-release-version.test.mjs index baf80a8cb..5e7a15619 100644 --- a/.github/scripts/set-release-version.test.mjs +++ b/.github/scripts/set-release-version.test.mjs @@ -1,4 +1,5 @@ -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { copyFileSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -107,9 +108,34 @@ describe("setReleaseVersion", () => { ); expect(() => setReleaseVersion("1.9.0", dir)).toThrow(/packages/); + + // The throw alone was never the property this test claims. package.json + // used to be written before the lockfile was validated, so this case left + // exactly the half-bump the name promises it prevents. + expect(JSON.parse(read("package.json")).version).toBe("1.8.0"); + expect(read("package-lock.json")).not.toContain("1.9.0"); }); it("requires a version", () => { expect(() => setReleaseVersion("", dir)).toThrow(/version is required/); }); + + // The workflows invoke this with a repository-relative path, and the + // direct-invocation guard compares against `import.meta.filename`, which is + // absolute. Node resolves argv[1] before exposing it, so the two match — but + // nothing pinned that, and the whole script is dead code if it ever stops + // being true. Invoked here the way promote.yml and prerelease.yml do. + it("runs when invoked directly through a relative path", () => { + const scripts = join(dir, "scripts"); + mkdirSync(scripts); + copyFileSync( + join(import.meta.dirname, "set-release-version.mjs"), + join(scripts, "set-release-version.mjs"), + ); + + execFileSync("node", ["scripts/set-release-version.mjs", "1.9.0"], { cwd: dir }); + + expect(JSON.parse(read("package.json")).version).toBe("1.9.0"); + expect(JSON.parse(read("package-lock.json")).packages[""].version).toBe("1.9.0"); + }); }); From 6dbdc477b7c909a9ae6c8beca0c39b89116100fc Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Sat, 8 Aug 2026 23:31:54 +0200 Subject: [PATCH 3/3] fix(release): reject a version that is not a release version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Truthiness alone let 123, " " and "not-a-version" reach both manifests; a number writes `"version": 123`, which is not a legal package.json. Narrower than semver on purpose — this gates what may be written into a published manifest, so build metadata or a leading v is a caller bug rather than a version to honour. Defence in depth rather than a live bug: both callers compute the version from an already-validated RC tag. Taken because a script whose purpose is to stop bad version metadata should not be the thing that writes it. --- .github/scripts/set-release-version.mjs | 15 +++++++++++++++ .github/scripts/set-release-version.test.mjs | 13 +++++++++++++ 2 files changed, 28 insertions(+) diff --git a/.github/scripts/set-release-version.mjs b/.github/scripts/set-release-version.mjs index 24f6d2315..1baf9ca4c 100644 --- a/.github/scripts/set-release-version.mjs +++ b/.github/scripts/set-release-version.mjs @@ -26,8 +26,23 @@ import { argv } from "node:process"; * @param {string} version Version to write, e.g. "1.9.0" or "1.9.0-rc.2". * @param {string} dir Directory holding package.json and package-lock.json. */ +// MAJOR.MINOR.PATCH with the optional prerelease suffix promote.yml and +// prerelease.yml actually produce ("1.9.0", "2.0.0-rc.3"). Deliberately not full +// semver: this is a gate on what may be written into a published manifest, and +// build metadata or a leading "v" would be a caller bug, not a version to honour. +const RELEASE_VERSION = /^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?$/; + export function setReleaseVersion(version, dir) { if (!version) throw new Error("a version is required"); + // Truthiness alone let 123, " " and "not-a-version" through into both + // manifests. The callers compute this from a validated tag, so it is a + // defence-in-depth check — but a script whose whole purpose is to stop bad + // version metadata should not be the thing that writes it. + if (typeof version !== "string" || !RELEASE_VERSION.test(version)) { + throw new Error( + `invalid version ${JSON.stringify(version)}; expected MAJOR.MINOR.PATCH with an optional prerelease suffix`, + ); + } // Both manifests are read and validated before a single byte is written. The // obvious order — write package.json, then validate the lockfile — left diff --git a/.github/scripts/set-release-version.test.mjs b/.github/scripts/set-release-version.test.mjs index 5e7a15619..77d69e64e 100644 --- a/.github/scripts/set-release-version.test.mjs +++ b/.github/scripts/set-release-version.test.mjs @@ -120,6 +120,19 @@ describe("setReleaseVersion", () => { expect(() => setReleaseVersion("", dir)).toThrow(/version is required/); }); + // Truthiness alone let all of these reach both manifests. A number in + // particular writes `"version": 123`, which is not even a legal package.json. + it.each([ + [123, "a number"], + [" ", "whitespace"], + ["not-a-version", "a malformed version"], + ["v1.9.0", "a leading v"], + ])("rejects %o (%s)", (bad) => { + expect(() => setReleaseVersion(bad, dir)).toThrow(/invalid version/); + expect(JSON.parse(read("package.json")).version).toBe("1.8.0"); + expect(read("package-lock.json")).not.toContain("1.9.0"); + }); + // The workflows invoke this with a repository-relative path, and the // direct-invocation guard compares against `import.meta.filename`, which is // absolute. Node resolves argv[1] before exposing it, so the two match — but