diff --git a/.github/workflows/release-sdk.yml b/.github/workflows/release-sdk.yml index 91709520b..ba0cc0fd1 100644 --- a/.github/workflows/release-sdk.yml +++ b/.github/workflows/release-sdk.yml @@ -627,6 +627,15 @@ jobs: rm -f "$TARBALL" - name: Dry-run publish validation + # Skip the rehearsal when the version is already on npm + # (reconciliation mode). `npm publish --dry-run` contacts the + # registry and fails with "You cannot publish over the + # previously published versions" if the version exists, even + # though no actual publish would be attempted. The real publish + # step (further down) is gated on the same condition; gate the + # rehearsal too so re-runs of an already-published hotfix don't + # fail here on a check that doesn't apply. Bug #2987. + if: ${{ steps.prior_publish.outputs.skip_publish != 'true' }} env: TAG: ${{ steps.ver.outputs.tag }} NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 567732407..4016d3245 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`release-sdk` hotfix re-run no longer fails at `Dry-run publish validation` when the version is already on npm** — the `Detect prior publish (reconciliation mode)` step sets `skip_publish=true` when the package version is already on the registry, and the actual publish step honors that gate. The `Dry-run publish validation` step was missing the same guard, so any operator re-run of an already-published hotfix (the typical recovery path when later steps fail mid-flight) hit `npm publish --dry-run` first and got `npm error You cannot publish over the previously published versions: X.Y.Z` — `npm publish --dry-run` contacts the registry and rejects existing-version targets even though it doesn't actually publish. The dry-run validation step is now gated on the same `steps.prior_publish.outputs.skip_publish != 'true'` condition as the publish step. The rehearsal still runs on first publishes (where it has value); it skips only in the specific reconciliation case where the publish itself would be skipped. Trigger run: [25233855236](https://github.com/gsd-build/get-shit-done/actions/runs/25233855236/job/73995605643). Regression covered by `tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs`. (#2987) - **`release-sdk` hotfix flow hardened against silent classifier failures, missing-classifier-at-base-tag, and a vestigial merge-back PR step** — three issues surfaced by CodeRabbit's post-merge review of #2981 plus a production failure on the v1.39.1 release run. **(1)** `scripts/diff-touches-shipped-paths.cjs` reused exit code `1` for both the legitimate "no shipped paths" classifier result and Node's default uncaught-throw exit, so any tooling failure was indistinguishable from a normal skip. The script now uses `0` (shipped), `1` (not shipped), `2` (classifier error) with `try`/`catch` + `uncaughtException`/`unhandledRejection` handlers routing all failure paths to exit `2`. **(2)** The workflow's `git checkout -b "$BRANCH" "$BASE_TAG"` overwrote the working tree with the base tag's contents *before* the cherry-pick loop ran the classifier — but base tags predating the classifier's introduction (notably v1.39.0) don't have the file in their tree, so `node scripts/diff-touches-shipped-paths.cjs` would exit non-zero and silently drop every commit, producing an empty hotfix release. The classifier is now staged into `$RUNNER_TEMP` at the top of `Prepare hotfix branch` (before any working-tree-mutating git command), and the loop references that staged copy. The cherry-pick loop snapshots `$PIPESTATUS` into a local array (`PIPE_RC=("${PIPESTATUS[@]}")`) immediately after the classifier pipeline — under bracketed `set +e`/`set -e` — and dispatches via explicit `case`: `0` proceeds, `1` skips into `NON_SHIPPED_SKIPPED`, anything else emits `::error::shipped-paths classifier failed for $SHA (exit N)` and fails the workflow. CodeRabbit on PR #2984 caught a subtler bug in the first iteration: `pipeline \|\| true; RC=${PIPESTATUS[1]}` is broken because `\|\| true` runs `true` as its own one-command pipeline on the failure paths, overwriting `PIPESTATUS` to `(0)` and leaving `${PIPESTATUS[1]}` unset. The array-snapshot form is invariant against this. The same hardening also surfaces `git diff-tree`'s exit code (via `PIPE_RC[0]`); a non-zero diff-tree result now also fails the workflow rather than feeding partial input to the classifier. **(3)** Removed the `Open merge-back PR (hotfix only)` step. The auto-cherry-pick hotfix flow only picks commits already on main (`git cherry HEAD origin/main` outputs the unmerged ones), so by construction every code commit on the hotfix branch is already on main. The only hotfix-branch-only commit is the version-bump chore, which would either no-op against main or rewind main's in-progress version. The step also failed in production with `GitHub Actions is not permitted to create or approve pull requests (createPullRequest)` (org policy) on run [25232968975](https://github.com/gsd-build/get-shit-done/actions/runs/25232968975). The `pull-requests: write` permission previously granted to the release job has been dropped in line with least-privilege. The run-summary line that previously echoed `Merge-back PR opened against main` has been replaced with `No merge-back PR (auto-picked commits are already on main)` so operators reading the summary see an accurate non-action statement (CodeRabbit on PR #2984). Regression covered by `tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs` (15 assertions across exit-code semantics, classifier staging, error dispatch, PIPESTATUS-snapshot hardening, diff-tree fail-fast, merge-back removal, and run-summary accuracy). (#2983) - **`release-sdk` hotfix only cherry-picks commits that change what actually ships** — the `fix:`/`chore:` filter in `Prepare hotfix branch` was too broad: it picked any commit with that conventional-commit type regardless of whether the diff could affect the published npm package. CI-only fixes (release-sdk.yml itself, hotfix tooling, test-only commits) were getting cherry-picked into hotfix branches even though they cannot change the tarball — and the subset touching `.github/workflows/*` then caused the prepare job's `git push` to be rejected by GitHub because the default `GITHUB_TOKEN` lacks the `workflow` scope, aborting the run. v1.39.1 hit this on PR #2977 (run [25232010071](https://github.com/gsd-build/get-shit-done/actions/runs/25232010071)). The loop now pre-skips any candidate commit whose `git diff-tree` output doesn't intersect the npm tarball's shipped paths (entries in `package.json` `files`, plus `package.json` itself, which `npm pack` always includes). Skipped commits land in a new `NON_SHIPPED_SKIPPED` summary bucket framed as informational — non-shipping commits cannot affect the package, so the skip needs no operator action. The shipped-paths classifier lives in `scripts/diff-touches-shipped-paths.cjs` so its rules (file-OR-directory prefix matching `npm pack` semantics, the always-shipped rule for `package.json`, the lockfile-not-shipped rule) are unit-testable. Regression covered by `tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs`. (#2980) - **`release-sdk` hotfix workflow fails on real run with `npm error Version not changed`** — the `release` job's `Bump in-tree version (not committed)` step ran `npm version "$VERSION"` without `--allow-same-version`, so it errored on real (non-dry-run) hotfix runs because `prepare` had already committed the bump on the hotfix branch. The release job's checkout `ref` is asymmetric — `BRANCH` (already bumped) on real runs vs `BASE_TAG` (older version) on dry-runs — which is why dry-run never caught the bug. Both `npm version` calls in that step now pass `--allow-same-version`, matching the existing pattern in `release.yml:326`. (#2976) diff --git a/tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs b/tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs new file mode 100644 index 000000000..6575183d4 --- /dev/null +++ b/tests/bug-2987-dry-run-validation-skip-on-reconciliation.test.cjs @@ -0,0 +1,140 @@ +/** + * Regression test for bug #2987 + * + * The release-sdk workflow's `Dry-run publish validation` step ran + * `npm publish --dry-run --tag "$TAG"` unconditionally. `npm publish + * --dry-run` contacts the registry and exits 1 when the version is + * already published: + * + * npm error You cannot publish over the previously published + * versions: 1.39.1. + * + * The earlier `Detect prior publish (reconciliation mode)` step + * already detects this case and sets + * `steps.prior_publish.outputs.skip_publish=true` — and the real + * publish step at line ~648 is gated on that. The dry-run validation + * was missing the same gate, so re-runs of an already-published + * hotfix (the operator's typical recovery path when a later step + * like merge-back fails) blew up at the rehearsal before reaching + * any of the reconciliation logic. + * + * Trigger run: 25233855236 — re-attempted v1.39.1 hotfix after the + * prior run had landed v1.39.1 on npm. + * + * Fix: gate the dry-run validation step on + * `steps.prior_publish.outputs.skip_publish != 'true'`, matching the + * publish step. + */ + +'use strict'; + +// allow-test-rule: source-text-is-the-product +// release-sdk.yml IS the product for hotfix automation; the assertions +// extract the workflow text and check the step-level `if:` guard via +// indentation-aware YAML parsing rather than raw-text grep. + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const WORKFLOW_PATH = path.join(__dirname, '..', '.github', 'workflows', 'release-sdk.yml'); + +/** + * Find a step by name and return the lines belonging to it (from the + * `- name:` line up to but not including the next `- name:` at the + * same indent or the next dedent-back-to-job). + */ +function extractStepBlock(workflowText, stepName) { + const lines = workflowText.split('\n'); + for (let i = 0; i < lines.length; i++) { + const m = lines[i].match(/^(\s*)- name:\s*(.+?)\s*$/); + if (!m || m[2] !== stepName) continue; + const stepIndent = m[1].length; + const start = i; + let end = lines.length; + for (let j = i + 1; j < lines.length; j++) { + const peek = lines[j]; + if (peek.length === 0) continue; + const lead = peek.match(/^(\s*)/)[1].length; + // Next sibling step or dedent past step indent terminates this block. + if (lead <= stepIndent && peek.trim().length > 0) { + if (/^\s*- /.test(peek) || lead < stepIndent) { + end = j; + break; + } + } + } + return lines.slice(start, end).join('\n'); + } + throw new Error(`step "${stepName}" not found in workflow`); +} + +describe('bug-2987: dry-run publish validation skips when reconciliation mode is active', () => { + test('Dry-run publish validation step has an `if:` guard tied to skip_publish', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const block = extractStepBlock(yaml, 'Dry-run publish validation'); + + // The guard must reference steps.prior_publish.outputs.skip_publish + // — the exact output set by the `Detect prior publish` step. + // Loosely accepting any boolean expression here would risk a future + // edit that gates on the wrong signal (e.g., inputs.dry_run, which + // is the user-facing dry-run flag, not registry reconciliation). + assert.match( + block, + /^\s*if:\s*\$\{\{\s*steps\.prior_publish\.outputs\.skip_publish\s*!=\s*'true'\s*\}\}\s*$/m, + "Dry-run publish validation must be gated on `steps.prior_publish.outputs.skip_publish != 'true'` so reconciliation re-runs (version already on npm) don't fail at the rehearsal (#2987)" + ); + }); + + test('the gate matches the actual publish step\'s gate (consistency with downstream skip)', () => { + // The publish step ("Publish to npm (CC bundle, ...)" further + // down) ALSO honors skip_publish. The rehearsal must honor it too; + // otherwise reconciliation runs always fail at the rehearsal. + // This test reads both gates and asserts the skip_publish + // sub-expression is identical between them. It allows the publish + // step to ALSO check inputs.dry_run (which it does, and which the + // rehearsal correctly does NOT — the rehearsal is the dry-run). + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const dryRunBlock = extractStepBlock(yaml, 'Dry-run publish validation'); + const publishBlock = extractStepBlock(yaml, 'Publish to npm (CC bundle, SDK included as both loose tree and .tgz)'); + + const skipPattern = /steps\.prior_publish\.outputs\.skip_publish\s*!=\s*'true'/; + assert.match( + dryRunBlock, + skipPattern, + 'Dry-run validation must check skip_publish (#2987)' + ); + assert.match( + publishBlock, + skipPattern, + 'Publish step must check skip_publish (sentinel — if this fails the workflow has changed and the test\'s premise needs review)' + ); + }); + + test('the workflow still runs the rehearsal in normal flows (gate is skip-only, not always-skip)', () => { + // Defense against the wrong fix: someone could pass-through-fix + // this by gating on `false` (always skip) which would silently + // disable the rehearsal even on first publishes. The gate must + // be specifically tied to the skip_publish signal, not a generic + // `false` or `inputs.action == 'something'` discriminator. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const block = extractStepBlock(yaml, 'Dry-run publish validation'); + + // The gate string itself must contain a comparison against 'true' — + // i.e., it's an opt-out for the prior-publish case, not an + // unconditional skip. + const ifLine = block.split('\n').find((l) => /^\s*if:/.test(l)); + assert.ok(ifLine, 'Dry-run validation must have an `if:` line (#2987)'); + assert.match( + ifLine, + /skip_publish\s*!=\s*'true'/, + 'gate must be `skip_publish != true` (run when not skipping), not an unconditional skip — the rehearsal still has value on first publishes (#2987)' + ); + assert.doesNotMatch( + ifLine, + /:\s*false\s*\}\}/, + 'gate must not be `if: false` — the rehearsal is meaningful when the version isn\'t yet on npm (#2987)' + ); + }); +});