From 9386fce14acaa8eb42d80efeb135abf415936081 Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 25 Aug 2026 09:41:51 +0000 Subject: [PATCH 01/11] fix: rank and detect prereleases the way the tags are actually written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isPrerelease` required the marker to follow a digit IMMEDIATELY, so every hyphenated semver spelling (`1.1.0-beta.1`, `2.0.0-rc.1`, `v1.2.3-alpha`) read as stable — and `git tag --sort=-v:refname` independently ranks such a tag ABOVE the release it qualifies. Together that made `preferredVersion` return the beta, so `stable/` aliased it and the root redirect pointed at it. Widen the marker set (PEP 440 + the hyphenated/dotted forms + the long spellings) and keep the digit anchor, so `release-1.0` and `beta-program` stay stable; a trailing lookahead keeps `1.0-candidate` out too. Order the tags here instead of in git: `compareTags` compares digit runs numerically and ranks a prerelease below its own release, so "what order" and "what counts as a prerelease" share one marker list rather than needing `versionsort.suffix` kept in step by hand. release.yml's `--prerelease` test carried the same rule in `case` globs, which are case-sensitive where the regex is not (`1.0.0RC1` was published stable). It now uses the identical marker list via `grep -Eqi`, and a new harness test extracts both implementations and cross-checks 27 tag spellings through each — the drift guard the previous "mirrors assemble.mjs" comment was standing in for. Also stop hardcoding main/master while we are in the ordering: the engine already knows the default branch, so thread it through `generate --default-branch`. A consumer whose default branch is `develop` had it sorted into the leftover bucket among the pr- previews, and it could never be the preferred version. Co-Authored-By: Claude Opus 5 --- .github/workflows/publish-gh-pages.yml | 3 +- .github/workflows/release.yml | 20 ++- assemble/assemble.mjs | 144 ++++++++++++++---- package.json | 2 +- test/test-assemble.mjs | 96 ++++++++++++ .../test_release_prerelease.py | 74 +++++++++ 6 files changed, 305 insertions(+), 34 deletions(-) create mode 100644 test/workflow-harness/test_release_prerelease.py diff --git a/.github/workflows/publish-gh-pages.yml b/.github/workflows/publish-gh-pages.yml index de84248..6d34084 100644 --- a/.github/workflows/publish-gh-pages.yml +++ b/.github/workflows/publish-gh-pages.yml @@ -446,7 +446,8 @@ jobs: run: | set -euo pipefail stable_src=$(node "$GITHUB_WORKSPACE/.mvs/assemble/assemble.mjs" generate \ - --site-dir "$RUNNER_TEMP/site" --base-url "$PAGES_URL" --required "$DEFAULT") + --site-dir "$RUNNER_TEMP/site" --base-url "$PAGES_URL" \ + --default-branch "$DEFAULT" --required "$DEFAULT") # An `&& ln … || true` one-liner here would also swallow a FAILED symlink, not # just the empty-stable_src case — and a missing stable/ alias is silent damage. if [ -n "$stable_src" ]; then diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 260cef3..bbd548f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -41,11 +41,23 @@ jobs: TAG: ${{ github.ref_name }} run: | set -euo pipefail - # Flag PEP 440-style prerelease tags — a/b/rc following a digit, e.g. - # 1.0a1 / 2.0rc1 (mirrors assemble.mjs's isPrerelease). Anchoring on the - # digit keeps tags that merely contain those letters out. + # Flag a prerelease tag — a marker following a digit (with an optional + # separator), so 1.0a1 / 2.0rc1 / 1.1.0-beta.1 / 2.0.0-rc.1 are prereleases + # while release-1.0 and 1.0-candidate are not. + # + # `markers` is assemble.mjs's MARKERS list VERBATIM, and the two are held + # together by a drift test (test/workflow-harness/test_release_prerelease.py) + # that runs a fixture list through both implementations — a comment alone had + # already let them diverge (this test was case-sensitive; the regex is not). + # There is no shared implementation to call: this workflow runs in the + # consumer's repo and deliberately checks nothing out. + markers='alpha|beta|preview|pre|rc|dev|a|b|c' prerelease="" - case "$TAG" in *[0-9]a*|*[0-9]b*|*[0-9]rc*) prerelease="--prerelease" ;; esac + # grep -E has no lookahead, so the trailing [^a-z] is spelled as a consumed + # character (or end-of-string); -i covers both cases, which `case` globs did not. + if printf '%s' "$TAG" | grep -Eqi "[0-9][._-]?($markers)([^a-z]|\$)"; then + prerelease="--prerelease" + fi if gh release view "$TAG" --repo "$REPO" >/dev/null 2>&1; then # The Release already exists (e.g. published from the UI on a mutable repo). diff --git a/assemble/assemble.mjs b/assemble/assemble.mjs index fe0052c..3b0bf18 100644 --- a/assemble/assemble.mjs +++ b/assemble/assemble.mjs @@ -21,7 +21,8 @@ * (the default branch's, plus each eligible open PR's), ready for * `xargs -0 -n 3`; a decision for every candidate goes to stderr. * - * node assemble.mjs generate --site-dir --base-url [--required ] + * node assemble.mjs generate --site-dir --base-url + * [--default-branch ] [--required ] * → write switcher.json + index.html into ; print the stable-alias * source dir (the newest deployed release) on stdout, or nothing. Also * exit-1s if a --required branch is absent from the site. @@ -100,27 +101,36 @@ export function discoverVersions(siteDir) { } /** - * Tags newest-first (semver-aware), matching `git tag -l --sort=-v:refname`. + * Tags newest-first, ordered by `compareTags` (NOT `git tag --sort=-v:refname`). * Tags containing `/` are dropped: the build trigger (`tags: ['*']`) never builds * them, so they have no matching `BASE_URL` build and would only create nested * site dirs. Every other tag is used verbatim as its `site/` dir name. + * + * git's version sort is not usable here: without a `versionsort.suffix` entry per + * marker it ranks `1.1.0-beta.1` ABOVE `1.1.0`, and that list would have to be kept + * in step with `isPrerelease` by hand. Sorting here keeps "what order" and "what + * counts as a prerelease" in one module, sharing one marker list. */ export function getSortedTags() { - return gitLines(["tag", "-l", "--sort=-v:refname"]).filter( - (tag) => !tag.includes("/"), - ); + return orderTags(gitLines(["tag", "-l"]).filter((tag) => !tag.includes("/"))); } /** - * Order the gathered versions: `master`, `main`, then tags newest-first, then any - * leftover directories (e.g. PR previews) alphabetically. `tags` must already be - * newest-first; `builds` are the directory names under `site/`. + * Order the gathered versions: the default branch, then `master`/`main` if they are + * present and distinct from it (a repo mid-rename may carry both), then tags + * newest-first, then any leftover directories (e.g. PR previews) alphabetically. + * `tags` must already be newest-first; `builds` are the directory names under `site/`. + * + * `defaultBranch` is optional so callers that have no opinion keep the historical + * `master`/`main` behaviour: a consumer whose default branch is `develop` would + * otherwise land it in the leftover bucket, sorted among the `pr-` previews. */ -export function orderVersions(builds, tags) { +export function orderVersions(builds, tags, defaultBranch) { const remaining = new Set(builds); const versions = []; - for (const version of ["master", "main", ...tags]) { + for (const version of [defaultBranch, "master", "main", ...tags]) { + if (!version) continue; if (remaining.has(version)) { versions.push(version); remaining.delete(version); @@ -346,26 +356,96 @@ export function selectArtifacts( } /** - * Is `tag` a prerelease? Mirrors `release.yml`'s test (a PEP 440-style `a`, - * `b`, or `rc` marker *following a digit*, e.g. `1.0a1`/`2.0rc1`) so "stable" - * means the same thing repo-wide. Anchoring on the digit keeps tags that merely - * contain those letters (`release-1.0`, `beta-program`) out of the prerelease set. + * The prerelease markers, longest-first so `preview` is not shadowed by `pre`. + * ONE list, used three ways: to test a tag (`isPrerelease`), to rank a prerelease + * below the release it qualifies (`compareTags`), and — as the identical literal — + * by release.yml's `--prerelease` test, which `test_release_prerelease.py` holds to + * this file. Covers PEP 440 (`1.0a1`, `2.0rc1`, `1.0.dev0`) and the hyphenated + * semver spellings (`1.1.0-beta.1`, `2.0.0-rc.1`). + */ +const MARKERS = "alpha|beta|preview|pre|rc|dev|a|b|c"; + +/** + * A marker anywhere in the tag, immediately after a digit (with an optional `.`, + * `-` or `_` between). The digit anchor keeps tags that merely contain those + * letters (`release-1.0`, `beta-program`) out of the prerelease set; the trailing + * lookahead keeps a longer word (`1.0-candidate`, `1.0-canary`) out of it too. + */ +const PRERELEASE_RE = new RegExp(`\\d[._-]?(${MARKERS})(?![a-z])`, "i"); + +/** The same marker, but anchored at the START of a version's trailing segment. */ +const PRERELEASE_SUFFIX_RE = new RegExp(`^[._-]?(${MARKERS})(?![a-z])`, "i"); + +/** + * Is `tag` a prerelease? Mirrors `release.yml`'s `--prerelease` test so "stable" + * means the same thing repo-wide — the two are held together by a drift test + * (`test/workflow-harness/test_release_prerelease.py`) rather than by comment alone. */ export function isPrerelease(tag) { - return /\d(a|b|rc)/i.test(tag); + return PRERELEASE_RE.test(tag); +} + +/** A tag split into runs of digits and non-digits, for segment-wise comparison. */ +function tagTokens(tag) { + // A leading `v` is a spelling of the same version (`v1.2.3` ≡ `1.2.3`), so drop + // it — but only before a digit, or `version-2` would become `ersion-2`. + return ( + String(tag) + .replace(/^v(?=\d)/i, "") + .match(/\d+|\D+/g) ?? [] + ); +} + +/** + * Compare two tags, NEWEST FIRST (so it sorts descending), with one invariant that + * `git tag --sort=-v:refname` does not give us: **every prerelease of X sorts after + * X**. Digit runs compare numerically (so `1.10` > `1.9`), other runs as strings. + * + * When one tag is the other plus a trailing segment, that segment decides: a + * prerelease marker (`1.1.0` vs `1.1.0-beta.1`) demotes the longer tag, anything + * else (`1.1` vs `1.1.1`) promotes it. Ties fall back to the raw string, so the + * order is total — an unstable ranking would change the published set on identical + * inputs. + */ +export function compareTags(a, b) { + const ta = tagTokens(a); + const tb = tagTokens(b); + for (let i = 0; i < Math.min(ta.length, tb.length); i += 1) { + if (ta[i] === tb[i]) continue; + const na = /^\d+$/.test(ta[i]); + const nb = /^\d+$/.test(tb[i]); + if (na && nb) return Number(tb[i]) - Number(ta[i]); + return ta[i] < tb[i] ? 1 : -1; + } + if (ta.length !== tb.length) { + const longer = ta.length > tb.length ? ta : tb; + const rest = longer.slice(Math.min(ta.length, tb.length)).join(""); + const demoted = PRERELEASE_SUFFIX_RE.test(rest); + // `sign` is +1 when `a` is the longer tag: demoted ⇒ a sorts later. + const sign = ta.length > tb.length ? 1 : -1; + return demoted ? sign : -sign; + } + return String(a).localeCompare(String(b)); +} + +/** Tags newest-first by `compareTags`. Pure; `getSortedTags` is the IO wrapper. */ +export function orderTags(tags = []) { + return [...tags].sort(compareTags); } /** * The preferred (stable) version: the newest non-prerelease tag that is actually - * deployed, else `main`, else `master`, else the first version. `tags` must be - * newest-first; `versions` is the deployed set (output of `orderVersions`). + * deployed, else the default branch, else `main`, else `master`, else the first + * version. `tags` must be newest-first; `versions` is the deployed set (output of + * `orderVersions`). `defaultBranch` is optional — see `orderVersions`. */ -export function preferredVersion(versions, tags) { +export function preferredVersion(versions, tags, defaultBranch) { for (const tag of tags) { if (!isPrerelease(tag) && versions.includes(tag)) return tag; } - if (versions.includes("main")) return "main"; - if (versions.includes("master")) return "master"; + for (const branch of [defaultBranch, "main", "master"]) { + if (branch && versions.includes(branch)) return branch; + } return versions[0] ?? null; } @@ -374,7 +454,7 @@ export function preferredVersion(versions, tags) { * * When `preferred` is a genuine deployed non-prerelease *tag*, the site publishes * a `stable/` alias pointing at it and the root redirects to the constant - * `stable/` URL. Before the first release (preferred is `main`/`master`/a + * `stable/` URL. Before the first release (preferred is the default branch or a * leftover, or a prerelease-only fallback) there is no `stable/` and the root * redirects straight to that fallback version. * @@ -382,8 +462,8 @@ export function preferredVersion(versions, tags) { * `stableSrc` is the version dir to alias as `stable/` (or null); `redirectTarget` * is the dir the root `index.html` should redirect to. */ -export function stablePlan(versions, tags) { - const preferred = preferredVersion(versions, tags); +export function stablePlan(versions, tags, defaultBranch) { + const preferred = preferredVersion(versions, tags, defaultBranch); if (preferred && tags.includes(preferred) && !isPrerelease(preferred)) { return { preferred, stableSrc: preferred, redirectTarget: STABLE_ALIAS }; } @@ -610,7 +690,7 @@ function cmdSelectArtifacts(rest) { } /** - * `generate --site-dir --base-url [--required ]` — write switcher.json + + * `generate --site-dir --base-url [--default-branch ] [--required ]` — write switcher.json + * index.html and emit the stable-alias source. Runs after all gathering, so it * also hard-fails (exit 1) if a `--required` branch is absent from the site. */ @@ -620,6 +700,7 @@ function cmdGenerate(rest) { options: { "site-dir": { type: "string" }, "base-url": { type: "string" }, + "default-branch": { type: "string" }, required: { type: "string" }, }, }); @@ -627,7 +708,7 @@ function cmdGenerate(rest) { const baseUrl = values["base-url"]; if (!siteDir || !baseUrl) { throw new Error( - "usage: assemble.mjs generate --site-dir --base-url [--required ]", + "usage: assemble.mjs generate --site-dir --base-url [--default-branch ] [--required ]", ); } @@ -646,10 +727,17 @@ function cmdGenerate(rest) { // Tags are used verbatim as site dirs (getSortedTags drops `/`-tags), so // ordering + preferred + stable compare directly against the discovered dirs. + // The default branch is not assumed to be main/master: it heads the switcher and + // is the preferred version until a release is deployed. + const defaultBranch = values["default-branch"]; const tags = getSortedTags(); - const versions = orderVersions(builds, tags); - const preferred = preferredVersion(versions, tags); - const { stableSrc, redirectTarget } = stablePlan(versions, tags); + const versions = orderVersions(builds, tags, defaultBranch); + const preferred = preferredVersion(versions, tags, defaultBranch); + const { stableSrc, redirectTarget } = stablePlan( + versions, + tags, + defaultBranch, + ); // Diagnostics go to stderr so stdout carries only the stable-alias source. console.error(`Sorted versions: ${JSON.stringify(versions)}`); diff --git a/package.json b/package.json index dc5f771..69999bc 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "test": "node test/test-url-logic.mjs && node test/test-assemble.mjs", "docs": "cd docs && myst build --html --strict", "docs-dev": "cd docs && myst start", - "test:workflows": "uv run --quiet --with pyyaml python3 test/workflow-harness/test_gather.py && uv run --quiet --with pyyaml python3 test/workflow-harness/test_shape.py" + "test:workflows": "uv run --quiet --with pyyaml python3 test/workflow-harness/test_gather.py && uv run --quiet --with pyyaml python3 test/workflow-harness/test_shape.py && uv run --quiet --with pyyaml python3 test/workflow-harness/test_release_prerelease.py" }, "files": [ "plugins/version-switcher.mjs", diff --git a/test/test-assemble.mjs b/test/test-assemble.mjs index 36231aa..ba78277 100644 --- a/test/test-assemble.mjs +++ b/test/test-assemble.mjs @@ -15,6 +15,7 @@ import { discoverVersions, isPrerelease, missingRequired, + orderTags, orderVersions, preferredVersion, RELZIPS_CACHE_PREFIX, @@ -74,6 +75,22 @@ assert.deepEqual(orderVersions(["pr-10", "pr-2", "main"], []), [ ]); ok("orderVersions sorts pr- previews numerically, not lexically"); +// a `develop` default branch heads the list instead of landing in the leftover +// bucket among the pr- previews. +assert.deepEqual(orderVersions(["pr-3", "2.1", "develop"], tags, "develop"), [ + "develop", + "2.1", + "pr-3", +]); +ok("orderVersions puts a named default branch first"); + +// main/master still come after it when a repo mid-rename carries both. +assert.deepEqual(orderVersions(["main", "develop"], [], "develop"), [ + "develop", + "main", +]); +ok("orderVersions keeps main after the named default branch"); + // --- discoverVersions: directory names under the site root --- const site = mkdtempSync(join(tmpdir(), "assemble-site-")); for (const d of ["main", "2.1", "2.0"]) mkdirSync(join(site, d)); @@ -100,6 +117,64 @@ assert.equal(isPrerelease("beta-program"), false); assert.equal(isPrerelease("stable-2.0"), false); ok("isPrerelease ignores a/b/rc letters not following a digit"); +// the hyphenated/dotted semver spellings, and the long marker words. These used to +// read as STABLE, which made `stable/` alias a beta. +assert.ok(isPrerelease("1.1.0-beta.1")); +assert.ok(isPrerelease("1.0.0-rc1")); +assert.ok(isPrerelease("v1.2.3-alpha")); +assert.ok(isPrerelease("2.0.0-pre.1")); +assert.ok(isPrerelease("1.0.dev0")); +assert.ok(isPrerelease("1.3.2-a9")); +ok("isPrerelease flags hyphenated semver + long-form markers"); + +// a longer word that merely STARTS with a marker is not one (the trailing lookahead). +assert.equal(isPrerelease("1.0-candidate"), false); +assert.equal(isPrerelease("2.0-canary"), false); +assert.equal(isPrerelease("1.0-preflight"), false); +ok("isPrerelease ignores longer words starting with a marker"); + +// --- compareTags/orderTags: newest first, prereleases below their own release --- +// THE invariant: for any tag X, every prerelease of X sorts after X. git's +// -v:refname does the opposite for the hyphenated forms. +assert.deepEqual( + orderTags([ + "1.0.0", + "1.1.0-beta.1", + "1.1.0", + "1.0.0-rc1", + "2.0.0a1", + "2.0.0", + ]), + ["2.0.0", "2.0.0a1", "1.1.0", "1.1.0-beta.1", "1.0.0", "1.0.0-rc1"], +); +ok("orderTags ranks each prerelease below the release it qualifies"); + +// digit runs compare numerically, so 1.10 is newer than 1.9 (not lexically before). +assert.deepEqual(orderTags(["1.9.0", "1.10.0", "1.9.1"]), [ + "1.10.0", + "1.9.1", + "1.9.0", +]); +ok("orderTags compares numeric segments numerically"); + +// a trailing segment that is NOT a prerelease marker means a newer version. +assert.deepEqual(orderTags(["1.1", "1.1.1"]), ["1.1.1", "1.1"]); +ok("orderTags treats a non-marker trailing segment as newer"); + +// a `v` prefix is a spelling of the same version, so it does not reorder anything. +assert.deepEqual(orderTags(["v1.0.0", "v1.2.0", "v1.1.0"]), [ + "v1.2.0", + "v1.1.0", + "v1.0.0", +]); +ok("orderTags ignores a leading v when comparing"); + +// total order: reordering the input cannot change the result (an unstable ranking +// would change the published set, and so the cache key, on identical inputs). +const shuffled = ["1.1.0-beta.1", "2.0.0", "1.0.0", "2.0.0a1", "1.1.0"]; +assert.deepEqual(orderTags(shuffled), orderTags([...shuffled].reverse())); +ok("orderTags is a total order (input order cannot change it)"); + // --- preferredVersion: newest deployed stable tag, else main --- assert.equal(preferredVersion(["main", "2.1", "2.0"], tags), "2.1"); ok("preferredVersion picks the newest deployed stable tag"); @@ -116,6 +191,19 @@ ok("preferredVersion falls back to main when no stable tag is deployed"); assert.equal(preferredVersion(["main", "2.0"], ["2.1", "2.0"]), "2.0"); ok("preferredVersion ignores tags with no deployed build"); +// the bug this pair guards: with the beta ranked above 1.1.0 by git's sort AND read +// as stable, preferredVersion returned the beta. +const semverTags = orderTags(["1.1.0", "1.1.0-beta.1"]); +assert.equal( + preferredVersion(["main", "1.1.0", "1.1.0-beta.1"], semverTags), + "1.1.0", +); +ok("preferredVersion prefers a release over its own hyphenated prerelease"); + +// a consumer whose default branch is neither main nor master. +assert.equal(preferredVersion(["develop", "pr-3"], [], "develop"), "develop"); +ok("preferredVersion falls back to the named default branch"); + // --- stablePlan: redirect target + stable-alias source --- // a deployed non-prerelease release → stable/ alias + root → stable/. assert.deepEqual(stablePlan(["main", "2.1", "2.0"], tags), { @@ -141,6 +229,14 @@ assert.deepEqual(stablePlan(["3.0rc1"], ["3.0rc1"]), { }); ok("stablePlan never aliases a prerelease as stable"); +// ...including the hyphenated spelling, which used to slip through as a real release. +assert.deepEqual(stablePlan(["main", "1.1.0-beta.1"], ["1.1.0-beta.1"]), { + preferred: "main", + stableSrc: null, + redirectTarget: "main", +}); +ok("stablePlan never aliases a hyphenated prerelease as stable"); + // --- missingRequired: required branches absent from the discovered site dirs --- // versions are the discovered site dirs; the default branch and every gathered // PR/preview are among them by generate time. Names compare verbatim (no rule). diff --git a/test/workflow-harness/test_release_prerelease.py b/test/workflow-harness/test_release_prerelease.py new file mode 100644 index 0000000..3f92431 --- /dev/null +++ b/test/workflow-harness/test_release_prerelease.py @@ -0,0 +1,74 @@ +""" +Drift guard: release.yml's `--prerelease` test vs assemble.mjs's isPrerelease. + +They are two implementations of one rule — bash in the consumer's repo (where nothing +of ours is checked out) and JS in the engine — and they HAVE drifted: the shell `case` +globs were case-sensitive while the regex carried /i, so `1.0.0RC1` was published as a +stable release but rendered as a prerelease. This runs a fixture list through both, the +bash half extracted from the workflow YAML so the test cannot go stale. +""" + +import os, re, subprocess, sys + +import yaml + +H = os.path.dirname(os.path.abspath(__file__)) +REPO = os.path.dirname(os.path.dirname(H)) + +# Every spelling the two implementations must agree on: PEP 440, hyphenated semver, +# the case variants, and the near-misses that must stay STABLE. +TAGS = [ + "1.0.0", "2.1", "v1.2.3", "1.10.0", + "1.0a1", "2.0rc1", "3.0b1", "1.0.0rc1", "1.0.0RC1", "1.0.0Rc1", + "1.1.0-beta.1", "1.0.0-rc1", "v1.2.3-alpha", "2.0.0-pre.1", "1.0.dev0", + "1.3.2-a9", "2.0.0-ALPHA", "1.0_b2", "1.0.0-preview.2", + "release-1.0", "beta-program", "stable-2.0", "1.0-candidate", "2.0-canary", + "1.0-preflight", "v2", "2026.03.1", +] + +fails = [] + + +def check(name, cond, detail=""): + print((" ok - " if cond else " FAIL - ") + name + (f"\n {detail}" if not cond and detail else "")) + if not cond: + fails.append(name) + + +def bash_body(): + """release.yml's prerelease test, lifted out of the step it lives in.""" + d = yaml.safe_load(open(f"{REPO}/.github/workflows/release.yml")) + step = next(s for s in d["jobs"]["release"]["steps"] if "Release with its assets" in s.get("name", "")) + body = step["run"] + # Everything up to the `gh release view` branch: the marker list and the test. + return body.split("if gh release view")[0] + + +BASH = bash_body() + +# One list in two files, so read both and compare the literals as well as the verdicts: +# equal behaviour on 27 fixtures would not stop someone adding a marker to only one. +wf_markers = re.search(r"markers='([^']*)'", BASH).group(1) +js_markers = re.search(r'const MARKERS = "([^"]*)"', open(f"{REPO}/assemble/assemble.mjs").read()).group(1) +check("release.yml carries assemble.mjs's MARKERS verbatim", wf_markers == js_markers, + f"release.yml={wf_markers!r} assemble.mjs={js_markers!r}") + +# The JS verdicts, one process for the whole list. +js = subprocess.run( + ["node", "--input-type=module", "-e", + f'import {{ isPrerelease }} from "{REPO}/assemble/assemble.mjs";' + f'for (const t of process.argv.slice(1)) console.log(isPrerelease(t) ? "1" : "0");', + "--", *TAGS], + capture_output=True, text=True) +assert js.returncode == 0, js.stderr +js_verdicts = js.stdout.split() + +for tag, want in zip(TAGS, js_verdicts): + p = subprocess.run(["bash", "-c", BASH + '\nprintf "%s" "${prerelease:-}"'], + env={**os.environ, "TAG": tag}, capture_output=True, text=True) + got = "1" if p.stdout.strip() == "--prerelease" else "0" + check(f"{tag}: both say {'prerelease' if want == '1' else 'stable'}", got == want, + f"release.yml={got} assemble.mjs={want} {p.stderr}") + +print(f"\n{'FAILURES: ' + ', '.join(fails) if fails else 'all prerelease-parity checks passed'}") +sys.exit(1 if fails else 0) From 0ada4eb845dc883b0a4c7d3caa8f09f5d9d3998d Mon Sep 17 00:00:00 2001 From: Tom Cobb Date: Tue, 25 Aug 2026 09:43:20 +0000 Subject: [PATCH 02/11] fix: keep context values out of docs.yml's shell, and validate the version name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit publish-gh-pages.yml documents the rule — every context value comes through `env:`, never interpolated into the script — but docs.yml's "Compute version name" step substituted `github.ref_name` straight into the body, where GitHub expands it as script text before bash parses it. Git accepts `$( )`, backticks and quotes in a ref name, and docs.yml is REUSABLE: a consumer triggering it on `push: branches: ['**']` hands that string, and so shell execution in their docs job, to anyone who can push a branch. The ref now comes through `env:`, and the resulting name is checked against `[A-Za-z0-9._-]` rather than assumed clean — the old comment's "no sanitisation" was only true given THIS repo's trigger set. A rejected name fails the build with the likely cause named. The general guard matters more than the fix: test_shape.py now walks every job of every workflow and fails on any `${{` inside a `run:` body, with no allow-list — those three lines were the only ones in the repo, so the rule starts clean. `renderRedirect` also escapes HTML metacharacters now. The version name it renders is validated one file away, and loosening that character set should not be able to inject markup here. Co-Authored-By: Claude Opus 5 --- .github/workflows/docs.yml | 32 +++++++++++++++++++++++++---- assemble/assemble.mjs | 26 +++++++++++++++++++---- test/test-assemble.mjs | 7 +++++++ test/workflow-harness/test_shape.py | 24 ++++++++++++++++++++++ 4 files changed, 81 insertions(+), 8 deletions(-) diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 50e1000..efc4dfc 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -62,21 +62,45 @@ jobs: # Version name = the site sub-dir this build is served at, and the BASE_URL it # must be built with: pr- for PRs, otherwise the ref name (the default branch, - # or a tag without `/`). No sanitisation — every version name is URL/path-safe. + # or a tag without `/`). # # This is the OTHER half of the contract, and it is not passed anywhere: the # gather independently re-derives the same name for each source it stages (a # release's tag, a PR's number, the repo's default_branch) and unpacks the zip at # site/. Both sides compute the same string from the same facts, so # this rule and assemble.mjs's dest rule must change together. + # + # The name is VALIDATED, not assumed clean. In THIS repo it is clean by + # construction (`tags: ['*']` never builds a `/`-tag), but this is a reusable + # workflow: a consumer triggering it on `push: branches: ['**']` would let anyone + # who can push a branch choose the string — and git accepts `$( )`, backticks and + # quotes in a ref name. Hence both halves here: the ref comes through `env:` so + # the shell never parses it as script text, and the character set is then checked + # so it stays a safe URL path segment and site dir. - name: Compute version name id: ver + env: + EVENT: ${{ github.event_name }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REF_NAME: ${{ github.ref_name }} run: | set -euo pipefail - if [ "${{ github.event_name }}" = pull_request ]; then - name="pr-${{ github.event.pull_request.number }}" + if [ "$EVENT" = pull_request ]; then + name="pr-$PR_NUMBER" else - name="${{ github.ref_name }}" + name="$REF_NAME" + fi + case "$name" in + "" | "." | "..") ok=false ;; + *[!A-Za-z0-9._-]*) ok=false ;; + *) ok=true ;; + esac + if [ "$ok" != true ]; then + echo "::error::version name '$name' is not usable as a URL path segment \ + — it must be non-empty and contain only A-Z a-z 0-9 . _ - (a '/' in a branch \ + or tag name is the usual cause). Build this ref under a name that is, or \ + exclude it from the workflow's triggers." + exit 1 fi echo "version-name=$name" >> "$GITHUB_OUTPUT" diff --git a/assemble/assemble.mjs b/assemble/assemble.mjs index 3b0bf18..b584ae4 100644 --- a/assemble/assemble.mjs +++ b/assemble/assemble.mjs @@ -508,16 +508,34 @@ export function renderSwitcher(baseUrl, versions, preferred) { return JSON.stringify(switcherStruct(baseUrl, versions, preferred), null, 2); } -/** Root redirect to `target` (relative, so it is host- and repo-agnostic). */ +/** Escape a value for an HTML attribute or text node. */ +function escapeHtml(value) { + return String(value) + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +} + +/** + * Root redirect to `target` (relative, so it is host- and repo-agnostic). + * + * `target` is a version name, which docs.yml validates down to `[A-Za-z0-9._-]` — so + * the escaping is belt-and-braces. It stays because this renders HTML from a value + * that ultimately derives from a ref name, and the two rules live in different files: + * loosening the workflow's character set should not be able to inject markup here. + */ export function renderRedirect(target) { + const safe = escapeHtml(target); return ` - Redirecting to ${target} + Redirecting to ${safe} - - + + diff --git a/test/test-assemble.mjs b/test/test-assemble.mjs index ba78277..4c4db45 100644 --- a/test/test-assemble.mjs +++ b/test/test-assemble.mjs @@ -312,6 +312,13 @@ const toMain = renderRedirect("main"); assert.match(toMain, /url=\.\/main\/index\.html/); ok("renderRedirect targets the fallback dir before the first release"); +// defence in depth: docs.yml validates the version name, but the two rules live in +// different files and this one renders HTML. +const nasty = renderRedirect('x">