From 49a5526ede8129f3320dfdcb6d77df11ac8134c4 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Sat, 3 Oct 2026 15:02:55 +0200 Subject: [PATCH] Release guard tidy: threat model stated, preload route pinned shut, dated changelog headings read - The npm Publish step starts the guard under `env -u NODE_OPTIONS`, and the pin's exact text carries it, so a workflow or job NODE_OPTIONS preload cannot run inside the guard (preload-fakes-guard). - The pin's shell/defaults ban and its test are gone: it defended only a deliberate edit, which is review's job (suite-gate-structural-not-parsed), and refused a harmless defaults.run.working-directory. A step-level shell still fails the exact comparison (shell-ban-deliberate-only). - docs/semver-policy.md states that the guard and pin stop accidental edits, that the cleaned environment removes known variables rather than proving no inherited one matters (env-denylist-overclaim), recommends a required-reviewer environment for the npm job, and gives the first-release note with the valid next versions. - check-changelog-semver accepts `## [x.y.z] - YYYY-MM-DD` and `## x.y.z - YYYY-MM-DD` beside the plain forms (changelog-first-entry). Co-Authored-By: Claude Opus 5.5 --- .github/workflows/publish-packages.yml | 5 ++- docs/semver-policy.md | 51 +++++++++++++++++++++---- scripts/check-changelog-semver.mjs | 7 ++-- scripts/check-changelog-semver.test.mjs | 32 ++++++++++------ 4 files changed, 70 insertions(+), 25 deletions(-) diff --git a/.github/workflows/publish-packages.yml b/.github/workflows/publish-packages.yml index b957bef9..dc4589a3 100644 --- a/.github/workflows/publish-packages.yml +++ b/.github/workflows/publish-packages.yml @@ -74,10 +74,11 @@ jobs: # One process checks that the tag is the manifest version, runs the # release-checks suite, the semver policy check (docs/semver-policy.md) # and npm test, and only then runs npm publish --access public. Only - # npm publish receives the token. + # npm publish receives the token. It starts without NODE_OPTIONS, so no + # preloaded module runs inside it. - name: Publish if: startsWith(github.ref, 'refs/tags/cultcache-ts-v') - run: node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts "$GITHUB_REF" + run: env -u NODE_OPTIONS node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts "$GITHUB_REF" env: NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/docs/semver-policy.md b/docs/semver-policy.md index b381da17..d1d9a3ed 100644 --- a/docs/semver-policy.md +++ b/docs/semver-policy.md @@ -126,7 +126,9 @@ considered stable), not something the checker infers on its own. Every published package keeps a `CHANGELOG.md` beside its manifest (`unity/org.gamecult.cultlib/CHANGELOG.md` and `src/GameCult.Unity/Assets/Caching/CHANGELOG.md` are the existing precedent). -Each release adds a `## []` section containing: +Each release adds a `## []` section (the check also reads +`## `, and either form followed by Keep a Changelog's +` - YYYY-MM-DD` date, such as `## [0.4.0] - 2026-10-03`) containing: - a category subsection per change (`### Added`, `### Changed`, `### Fixed`, etc., following Keep a Changelog style); @@ -140,6 +142,22 @@ A version with no changelog entry at all fails the check below, the same as a mislabelled one — an undocumented release is not a smaller sin than a mislabelled one. +### First release of a package + +A package with no `CHANGELOG.md` yet cannot release: the check refuses with +"changelog not found". Its first release writes the file at the declared path, +`packages//CHANGELOG.md`, with one section for the version being +released. That version must still be exactly the next one after the package's +nearest ancestor tag. As of 2026-10-03 the valid next versions are: + +- `cultcache-ts`, after `cultcache-ts-v0.14.0`: `0.14.1`, `0.15.0` or `1.0.0`; +- `cultcache-py`, after `cultcache-py-v0.3.0`: `0.3.1`, `0.4.0` or `1.0.0`; +- `cultnet-py` and `cultmesh-py`, which have no tags: `0.1.0`, their manifest + version, as a first release. Run it from a clone with the tags fetched, or + the check cannot tell a first release from a missing record. + +On `0.y.z` a `### Breaking` section needs at least a minor bump. + ## Enforcement `scripts/check-changelog-semver.mjs` is the mechanical check. Given a @@ -289,15 +307,32 @@ check for the tagged version, runs the package's own tests (`npm test`, or the Python package's unit tests), and only then the release action: `npm publish` for `cultcache-ts`, which it refuses on any ref but a release tag, and for the Python packages the sdist and wheel build, whose output is all the PyPI upload -step uploads. The checks run without `NODE_OPTIONS`, `NODE_TEST_CONTEXT`, any -other `NODE_TEST_*` variable and `NODE_AUTH_TOKEN`, so no inherited variable can -make the tests run nothing and pass; only the release action receives the token. +step uploads. The checks run with the known test-runner and token variables +removed (`NODE_OPTIONS`, `NODE_TEST_CONTEXT`, any other `NODE_TEST_*` variable, +and `NODE_AUTH_TOKEN`), so none of those can make the tests run nothing and +pass, and only the release action receives the token. That list names known +variables; it does not prove that no other inherited variable can affect a +check. The npm Publish step starts the guard under `env -u NODE_OPTIONS`, so a +`NODE_OPTIONS` preload set in the workflow's or job's `env:` cannot run code +inside the guard before its first line. The gate is that call, not a step condition, so no `if:` or `continue-on-error` can release what a check refused. The tests compare each job's release steps -with their expected text exactly, less the steps' `if:` lines, and refuse `shell` -and `defaults` anywhere in the workflow, which could run a release step's line -through another command. Removing the call is still possible, but only as a visible edit to those -steps and to the tests. +with their expected text exactly, less the steps' `if:` lines. Removing the call +is still possible, but only as a visible edit to those steps and to the tests. + +The guard and that pin stop accidental workflow edits. They do not stop a +deliberate one: an extra step that publishes, a workflow-level `defaults:` or +`env:`, or a fake `node` on `$GITHUB_PATH` all pass them. Deliberate edits are +review's job, as ruled in `suite-gate-structural-not-parsed`, and the pin +does not try to parse the workflow for them. For the same reason it has no ban +on `shell` or `defaults`: such a ban defends only against a deliberate edit, +and it also refused harmless settings such as `defaults.run.working-directory`. +A step-level `shell:` on a release step still fails the exact comparison. +A recommendation for the operator, not something this repository can set: the +Python job already runs in the `pypi` GitHub environment, as PyPI trusted +publishing expects. Putting the npm job in an environment with required +reviewers would make every npm release wait for a person's approval. That is a +GitHub settings change. `org.gamecult.cultlib` compiles CultMath from source and ships beside `org.gamecult.cultmath`, so its build refuses unless it declares exactly the diff --git a/scripts/check-changelog-semver.mjs b/scripts/check-changelog-semver.mjs index 80af1226..1d060419 100644 --- a/scripts/check-changelog-semver.mjs +++ b/scripts/check-changelog-semver.mjs @@ -73,12 +73,13 @@ export function classifyBump(prev, next) { return { invalid: compareVersions(next, prev) < 0 ? "reverse" : "skip" }; } -// Extracts the `## []` section of a changelog and reports whether it -// carries a BREAKING_HEADING subsection. Throws if the version has no entry +// Extracts the `## []` section of a changelog (also `## `, +// and either with Keep a Changelog's ` - YYYY-MM-DD` date) and reports whether +// it carries a BREAKING_HEADING subsection. Throws if the version has no entry // at all — an undocumented release is refused the same as a mislabelled one. export function hasBreakingSection(changelogText, version) { const sectionHeading = new RegExp( - `^##\\s*\\[?${version.replace(/\./g, "\\.")}\\]?\\s*$`, + `^##\\s*\\[?${version.replace(/\./g, "\\.")}\\]?(\\s+-\\s+\\d{4}-\\d{2}-\\d{2})?\\s*$`, "m", ); const startMatch = sectionHeading.exec(changelogText); diff --git a/scripts/check-changelog-semver.test.mjs b/scripts/check-changelog-semver.test.mjs index 72f2afdd..9d8424e0 100644 --- a/scripts/check-changelog-semver.test.mjs +++ b/scripts/check-changelog-semver.test.mjs @@ -84,6 +84,19 @@ test("hasBreakingSection scopes to the named version's section only", () => { assert.throws(() => hasBreakingSection(text, "0.9.0"), /no "## \[0\.9\.0\]" entry/); }); +test("hasBreakingSection reads Keep a Changelog's dated heading beside the plain forms", () => { + const text = ["## [0.4.0] - 2026-10-03", "### Breaking", "- x", "## 0.3.1 - 2026-09-01", "### Breaking", "## [0.3.0]", "### Fixed", ""].join("\n"); + assert.equal(hasBreakingSection(text, "0.4.0"), true); + assert.equal(hasBreakingSection(text, "0.3.1"), true); + assert.equal(hasBreakingSection(text, "0.3.0"), false); + // a dated section ends at the next heading, dated or not + assert.equal(hasBreakingSection(["## [0.4.0] - 2026-10-03", "### Fixed", "## [0.3.9] - 2026-09-01", "### Breaking"].join("\n"), "0.4.0"), false); + // only a date may follow the version + for (const heading of ["## [0.4.0] - soon", "## [0.4.0] 2026-10-03", "## [0.4.0] - 2026-10-03 extra", "## [0.4.0.1] - 2026-10-03"]) { + assert.throws(() => hasBreakingSection(`${heading}\n### Breaking\n`, "0.4.0"), /no "## \[0\.4\.0\]" entry/, heading); + } +}); + // Table-driven per the operator's required cases (spec: breaking+major=pass, // breaking+patch=fail, breaking+minor=fail, non-breaking+minor=pass, // 0.y.z breaking+minor=pass, skipped version=fail, downgrade=fail). @@ -746,16 +759,16 @@ test("workflow: the Caching Unity prefix is its own tag family, not another pack // suite, the semver policy check and the package's tests and then the release action from one process, so no // if: or continue-on-error on any step can release what those checks did not pass. This pin reads no // condition. It compares each job's release steps whole, less their own if: line, with the text below, so a -// release step that does anything else is a mismatch, and it refuses shell and defaults anywhere, which could -// run the pinned line through another command. Each Unity release script runs the suite itself, before it -// calls the checker. +// release step that does anything else is a mismatch. It stops accidental edits; a deliberate one, such as a +// workflow-level defaults or env or an extra step, is review's job (ruling suite-gate-structural-not-parsed). +// Each Unity release script runs the suite itself, before it calls the checker. const SUITE = /node\s+--test[^\n]*check-changelog-semver\.test\.mjs/; const CHECKER = /check-changelog-semver\.mjs/; const code = (text) => text.split("\n").filter((line) => !line.trim().startsWith("#")).join("\n"); const RELEASE_STEPS = { "cultcache-ts": [ - ["- name: Publish", " run: node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts \"$GITHUB_REF\"", " env:", " NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}"], + ["- name: Publish", " run: env -u NODE_OPTIONS node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts \"$GITHUB_REF\"", " env:", " NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}"], ], // Publish uploads only the dist the guarded step built, so it has nothing to upload unless the suite passed. python: [ @@ -805,10 +818,6 @@ function workflowReleaseProblems(workflow) { from = at + 1; } }); - // A job's or the workflow's defaults.run.shell would run the pinned release line through any command. - if (/\bshell\b|\bdefaults\b/.test(code(workflow))) { - problems.push("the workflow says shell or defaults: a release step must run its pinned line through the default shell"); - } return problems; } @@ -821,7 +830,7 @@ function scriptSuiteProblems(script) { test("workflow: each job releases only through release-after-suite.mjs", () => { const workflow = workflowText(); assert.deepEqual(workflowReleaseProblems(workflow), []); - const npmRun = 'run: node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts "$GITHUB_REF"'; + const npmRun = 'run: env -u NODE_OPTIONS node scripts/release-after-suite.mjs npm-publish packages/cultcache-ts "$GITHUB_REF"'; const pythonRun = 'run: node scripts/release-after-suite.mjs python-build packages/${{ matrix.package }} "$GITHUB_REF"'; const exact = /is not exactly the release call/; for (const run of [npmRun, pythonRun]) { @@ -833,15 +842,14 @@ test("workflow: each job releases only through release-after-suite.mjs", () => { assert.match(edit("run: |\n node --test scripts/check-changelog-semver.test.mjs\n true"), exact, run); assert.match(edit(run.replace("release-after-suite.mjs", "check-changelog-semver.mjs")), exact, run); assert.match(edit(run.replace('"$GITHUB_REF"', "refs/tags/cultcache-ts-v0.14.0")), exact, run); - assert.match(edit(`${run}\n shell: bash`), /another command|default shell/, run); + assert.match(edit(`${run}\n shell: bash`), exact, run); assert.match(edit(`${run}\n continue-on-error: true`), exact, run); } // the guard runs every release check itself, so neither the release steps' conditions nor a job allowed // to fail can release what a check refused assert.deepEqual(workflowReleaseProblems(workflow.replace(/if: startsWith\(github\.ref, 'refs\/tags\/cultcache-ts-v'\)(\r?\n\s+run: node)/, "if: always()$1")), []); assert.deepEqual(workflowReleaseProblems(workflow.replace(/( runs-on: ubuntu-latest\r?\n)/, "$1 continue-on-error: true\n")), []); - // a job's default shell, a release step removed, renamed or doubled, a Publish that uploads another dist, an unknown job - assert.match(workflowReleaseProblems(workflow.replace(/( runs-on: ubuntu-latest\r?\n)/, "$1 defaults:\n run:\n shell: sh -c 'npm publish' {0}\n")).join("\n"), /default shell/); + // a release step removed, renamed or doubled, a Publish that uploads another dist, an unknown job assert.match(workflowReleaseProblems(workflow.replace("- name: Build sdist and wheel", "- name: Build")).join("\n"), /needs exactly one step "- name: Build sdist and wheel"/); assert.match(workflowReleaseProblems(workflow.replace("- name: Build and test", "- name: Publish\n run: npm publish\n\n - name: Build and test")).join("\n"), /needs exactly one step "- name: Publish"/); assert.match(workflowReleaseProblems(workflow.replace("packages-dir: packages/${{ matrix.package }}/dist", "packages-dir: dist")).join("\n"), exact);