diff --git a/.github/workflows/release-sdk.yml b/.github/workflows/release-sdk.yml index 8eeea2d20..91709520b 100644 --- a/.github/workflows/release-sdk.yml +++ b/.github/workflows/release-sdk.yml @@ -122,6 +122,23 @@ jobs: DRY_RUN: ${{ inputs.dry_run }} run: | set -euo pipefail + # Stash the shipped-paths classifier from the dispatched ref's + # working tree BEFORE `git checkout -b ... "$BASE_TAG"` below + # overwrites it. Base tags predating #2980 don't have the + # classifier in their tree, so the loop must reference a + # location that survives the working-tree swap. Bug #2983. + CLASSIFIER_SRC="scripts/diff-touches-shipped-paths.cjs" + if [ ! -f "$CLASSIFIER_SRC" ]; then + echo "::error::shipped-paths classifier not found at $CLASSIFIER_SRC in dispatched ref — refusing to run" + exit 1 + fi + CLASSIFIER="${RUNNER_TEMP}/diff-touches-shipped-paths.cjs" + cp "$CLASSIFIER_SRC" "$CLASSIFIER" + if [ ! -f "$CLASSIFIER" ]; then + echo "::error::failed to stage classifier at $CLASSIFIER" + exit 1 + fi + MAJOR_MINOR=$(echo "$VERSION" | cut -d. -f1-2) TARGET_TAG="v${VERSION}" BRANCH="hotfix/${VERSION}" @@ -213,13 +230,49 @@ jobs: # treat entries as file-OR-directory prefix, the # `package.json`-always-shipped rule) are # unit-testable. Bug #2980. - if ! git diff-tree --no-commit-id --name-only -r "$SHA" \ - | node scripts/diff-touches-shipped-paths.cjs; then - REASON="touches no shipped paths (CI / test / docs / planning only)" - echo "↷ skipping $SHA — $REASON" - NON_SHIPPED_SKIPPED="${NON_SHIPPED_SKIPPED}- \`${SHA}\` ${SUBJECT}"$'\n' - continue + # + # Use $CLASSIFIER (staged at workflow-start, before + # `git checkout -b ... "$BASE_TAG"` swapped the working + # tree) rather than `scripts/...` directly — base tags + # older than #2980 don't have the classifier in their + # tree. Capture the exit code via PIPESTATUS and + # dispatch on it: 0 = shipped, 1 = not shipped, 2+ = + # classifier error → fail-fast (don't silently treat + # tooling errors as informational skips). Bug #2983. + # + # PIPESTATUS capture must happen IMMEDIATELY after the + # pipeline — the previous form (`pipeline || true; RC= + # ${PIPESTATUS[1]}`) had a subtle bug: when the + # pipeline fails (exit 1 or 2 — exactly the cases we + # care about), `|| true` runs `true` as a one-command + # pipeline, overwriting PIPESTATUS to (0). The fix is + # to wrap the pipeline in `set +e`/`set -e` and snapshot + # PIPESTATUS into a local array on the very next line. + # CodeRabbit on PR #2984. + set +e + git diff-tree --no-commit-id --name-only -r "$SHA" \ + | node "$CLASSIFIER" + PIPE_RC=("${PIPESTATUS[@]}") + set -e + DIFFTREE_RC="${PIPE_RC[0]}" + CLASSIFIER_RC="${PIPE_RC[1]}" + if [ "$DIFFTREE_RC" -ne 0 ]; then + echo "::error::git diff-tree failed for $SHA (exit $DIFFTREE_RC) — refusing to classify on incomplete input." + exit "$DIFFTREE_RC" fi + case "$CLASSIFIER_RC" in + 0) ;; + 1) + REASON="touches no shipped paths (CI / test / docs / planning only)" + echo "↷ skipping $SHA — $REASON" + NON_SHIPPED_SKIPPED="${NON_SHIPPED_SKIPPED}- \`${SHA}\` ${SUBJECT}"$'\n' + continue + ;; + *) + echo "::error::shipped-paths classifier failed for $SHA (exit $CLASSIFIER_RC). Refusing to silently skip — bug #2983." + exit "$CLASSIFIER_RC" + ;; + esac echo "→ cherry-picking $SHA $SUBJECT" # Pin merge.conflictStyle=merge on the cherry-pick so the # awk classifier below sees deterministic marker shapes — @@ -385,9 +438,11 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 permissions: - contents: write # tag + push + GitHub Release - id-token: write # provenance - pull-requests: write # hotfix mode opens merge-back PR via gh pr create + contents: write # tag + push + GitHub Release + id-token: write # provenance + # The merge-back PR step (and the pull-request scope it required) + # was removed in #2983 — auto-cherry-pick hotfix flow only picks + # commits already on main, so there's nothing to merge back. environment: npm-publish steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -639,25 +694,22 @@ jobs: fi echo "✅ GitHub Release v${VERSION} ready" - - name: Open merge-back PR (hotfix only) - if: ${{ !inputs.dry_run && inputs.action == 'hotfix' }} - env: - GH_TOKEN: ${{ github.token }} - BRANCH: ${{ needs.prepare.outputs.ref }} - VERSION: ${{ steps.ver.outputs.version }} - run: | - EXISTING_PR=$(gh pr list --base main --head "$BRANCH" --state open --json number --jq '.[0].number') - if [ -n "$EXISTING_PR" ]; then - gh pr edit "$EXISTING_PR" \ - --title "chore: merge hotfix v${VERSION} back to main" \ - --body "Merge hotfix changes back to main after v${VERSION} release." - else - gh pr create \ - --base main \ - --head "$BRANCH" \ - --title "chore: merge hotfix v${VERSION} back to main" \ - --body "Merge hotfix changes back to main after v${VERSION} release." - fi + # Merge-back PR step removed — bug #2983. + # + # The auto-cherry-pick hotfix flow only picks commits already on + # main (`git cherry HEAD origin/main` outputs unmerged commits; + # we filter to fix:/chore: from main). By construction every code + # commit on the hotfix branch is already on main. The only + # hotfix-branch-only commit is `chore: bump version to X.Y.Z for + # hotfix`, which would either no-op against main (already past + # X.Y.Z) or rewind main's in-progress version — strictly + # counterproductive in either case. + # + # The original merge-back step also failed in production with + # `GitHub Actions is not permitted to create or approve pull + # requests (createPullRequest)` (org policy), but even if the + # policy were lifted the PR would have nothing useful to merge. + # Run 25232968975 was the trigger for removal. - name: Verify publish landed on registry if: ${{ !inputs.dry_run }} @@ -717,7 +769,12 @@ jobs: echo "- \`next\` dist-tag re-pointed at \`v${VERSION}\` (kept current with \`latest\`)" fi if [ "$ACTION" = "hotfix" ]; then - echo "- Merge-back PR opened against main" + # Auto-cherry-pick hotfixes only pick commits already on + # main, so there's nothing to merge back. The merge-back + # PR step was removed in #2983; this line surfaces the + # explicit non-action so operators don't expect a PR + # that was never opened. + echo "- No merge-back PR (auto-picked commits are already on main)" fi echo "- Install: \`npm install -g get-shit-done-cc@${TAG}\`" fi diff --git a/CHANGELOG.md b/CHANGELOG.md index fc47e3116..567732407 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 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) - **`gsd-sdk query agent-skills` emits raw `` block instead of JSON-wrapped string** — workflows that embed via `$(gsd-sdk query agent-skills )` were receiving a JSON-quoted string literal mid-prompt (e.g. `"\n…"`), silently breaking all `` injection into spawned subagents. The CLI dispatcher now honors an opt-in `format: 'text'` field on `QueryResult` and writes such results raw via `process.stdout.write`; `--pick` always returns JSON regardless. (#2917) diff --git a/scripts/diff-touches-shipped-paths.cjs b/scripts/diff-touches-shipped-paths.cjs index 1002e16e8..f91855885 100644 --- a/scripts/diff-touches-shipped-paths.cjs +++ b/scripts/diff-touches-shipped-paths.cjs @@ -5,8 +5,8 @@ * * Reads a newline-separated list of paths from stdin (typically the * output of `git diff-tree --no-commit-id --name-only -r `) and - * exits 0 if any path is part of the npm tarball's shipped contents, - * 1 otherwise. + * exits with one of three codes so the workflow can distinguish a + * legitimate "skip this commit" signal from a classifier failure. * * "Shipped" = the union of: * - package.json (always included by `npm pack`, regardless of `files`) @@ -17,10 +17,20 @@ * excludes it from the tarball unless it's explicitly in `files`, and at * the time of writing this repo's `files` whitelist does not include it. * - * Exit codes: - * 0 at least one path is shipped → cherry-pick is meaningful - * 1 no shipped paths → CI / test / docs / planning-only; - * hotfix loop skips the commit + * Exit codes (the workflow MUST treat these distinctly — bug #2983): + * 0 at least one path is shipped → cherry-pick is meaningful + * 1 no shipped paths → CI / test / docs / planning + * only; hotfix loop skips + * 2 classifier error → bad/missing package.json, + * I/O failure, or any + * uncaught exception. The + * workflow MUST fail-fast on + * this code rather than + * treating it as a skip. + * + * Why distinct codes: Node's default exit code for uncaught throws is 1, + * which would otherwise be indistinguishable from the legitimate "no + * shipped paths" result. CodeRabbit on PR #2981 / bug #2983. */ 'use strict'; @@ -28,6 +38,10 @@ const fs = require('node:fs'); const path = require('node:path'); +const EXIT_SHIPPED = 0; +const EXIT_NOT_SHIPPED = 1; +const EXIT_ERROR = 2; + function loadShipPrefixes(pkgPath) { const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); const files = Array.isArray(pkg.files) ? pkg.files : []; @@ -42,19 +56,41 @@ function isShipped(diffPath, shipPrefixes) { return shipPrefixes.some((s) => p === s || p.startsWith(s + '/')); } +function fail(message, err) { + process.stderr.write(`diff-touches-shipped-paths: ${message}\n`); + if (err && err.stack) process.stderr.write(`${err.stack}\n`); + process.exit(EXIT_ERROR); +} + function main() { - const pkgPath = path.resolve(process.cwd(), 'package.json'); - const shipPrefixes = loadShipPrefixes(pkgPath); + // Surface ANY uncaught failure as exit 2 (classifier error) rather + // than letting Node's default-1 shadow the legitimate + // "no shipped paths" result. Bug #2983. + process.on('uncaughtException', (err) => fail('uncaught exception', err)); + process.on('unhandledRejection', (err) => fail('unhandled rejection', err)); + + let shipPrefixes; + try { + const pkgPath = path.resolve(process.cwd(), 'package.json'); + shipPrefixes = loadShipPrefixes(pkgPath); + } catch (err) { + return fail(`failed to read package.json from ${process.cwd()}`, err); + } let buf = ''; process.stdin.setEncoding('utf8'); + process.stdin.on('error', (err) => fail('stdin read error', err)); process.stdin.on('data', (chunk) => { buf += chunk; }); process.stdin.on('end', () => { - const paths = buf.split('\n').map((s) => s.trim()).filter(Boolean); - const hit = paths.some((p) => isShipped(p, shipPrefixes)); - process.exit(hit ? 0 : 1); + try { + const paths = buf.split('\n').map((s) => s.trim()).filter(Boolean); + const hit = paths.some((p) => isShipped(p, shipPrefixes)); + process.exit(hit ? EXIT_SHIPPED : EXIT_NOT_SHIPPED); + } catch (err) { + fail('classification failed', err); + } }); } @@ -62,4 +98,4 @@ if (require.main === module) { main(); } -module.exports = { loadShipPrefixes, isShipped }; +module.exports = { loadShipPrefixes, isShipped, EXIT_SHIPPED, EXIT_NOT_SHIPPED, EXIT_ERROR }; diff --git a/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs b/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs index 66ea3b782..787d2d50c 100644 --- a/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs +++ b/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs @@ -73,13 +73,15 @@ describe('bug-2964: release-sdk hotfix cherry-pick survives empty commits', () = // The cherry-pick call lives within the auto_cherry_pick loop. Bound // the slice generously after the anchor so future pre-skip guards / // classification scaffolding (e.g. the merge-commit pre-skip added - // on PR #2970, the workflow-file pre-skip added on PR for #2980) - // don't push the call out of range, but still tight enough to avoid - // matching unrelated cherry-pick refs elsewhere in the workflow file. + // on PR #2970, the workflow-file pre-skip added on PR for #2980, + // the PIPESTATUS-snapshot hardening added on PR for #2984's CR + // findings) don't push the call out of range, but still tight + // enough to avoid matching unrelated cherry-pick refs elsewhere in + // the workflow file. // Allow arbitrary git options between `git` and `cherry-pick` (e.g. // `git -c merge.conflictStyle=merge cherry-pick ...` added for #2966) // so this test doesn't false-fail on legitimate option additions. - const window = yaml.slice(loopAnchor, loopAnchor + 6000); + const window = yaml.slice(loopAnchor, loopAnchor + 8000); const pickMatch = /git\b[^\n]*?cherry-pick[^\n]*"\$SHA"/.exec(window); assert.ok( pickMatch, diff --git a/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs b/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs index c61872422..1e0da0a43 100644 --- a/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs +++ b/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs @@ -123,18 +123,25 @@ describe('bug-2980: release-sdk hotfix only picks commits that touch shipped pat /git diff-tree --no-commit-id --name-only -r "\$SHA"/, 'pre-pick region must extract the candidate SHA\'s file list with `git diff-tree` so the classifier has accurate input (#2980)' ); + // After #2983 the classifier is invoked via the staged $CLASSIFIER + // variable (not the in-tree path), to survive the working-tree swap + // performed by `git checkout -b "$BRANCH" "$BASE_TAG"`. Either form + // proves the classifier participates; the bug-2983 test enforces + // the staged-path form specifically. assert.match( prePick, - /node scripts\/diff-touches-shipped-paths\.cjs/, - 'pre-pick region must invoke scripts/diff-touches-shipped-paths.cjs to decide whether the commit touches any shipped path (#2980)' + /node "\$CLASSIFIER"/, + 'pre-pick region must invoke `node "$CLASSIFIER"` (the staged classifier) — the in-tree path is unsafe after the base-tag checkout (#2980, #2983)' ); - // The conditional negates the script's exit code — exit 0 means at - // least one shipped path was touched (proceed), exit 1 means none - // (skip). `if !` is the only correct form. + // Skip-on-exit-1 dispatch: pre-#2983 used `if ! ... ; then skip`, + // but that conflated classifier errors (exit 2+) with the + // legitimate "not shipped" signal. Post-#2983 the dispatch is + // explicit `case "$CLASSIFIER_RC" in 1) skip ;; *) error ;; esac`. + // This test accepts the modern form; bug-2983 enforces it. assert.match( prePick, - /if ! git diff-tree[\s\S]*scripts\/diff-touches-shipped-paths\.cjs;/, - 'pre-pick region must skip on classifier exit 1 (no shipped paths) — `if ! ... ; then ... continue` is the required shape (#2980)' + /case "\$CLASSIFIER_RC" in[\s\S]+?1\)[\s\S]+?continue/, + 'pre-pick region must skip on exit 1 via case-dispatch on $CLASSIFIER_RC — the post-#2983 shape that distinguishes "not shipped" from classifier errors (#2980, #2983)' ); }); diff --git a/tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs b/tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs new file mode 100644 index 000000000..c6602d66d --- /dev/null +++ b/tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs @@ -0,0 +1,388 @@ +/** + * Regression test for bug #2983 + * + * Two compounding bugs surfaced by CodeRabbit's post-merge review of + * PR #2981 (which shipped #2980's shipped-paths cherry-pick filter): + * + * 1. Overloaded exit code: scripts/diff-touches-shipped-paths.cjs + * used exit 1 for the legitimate classifier result "no shipped + * paths." Node's default exit on uncaught throw is also 1, so any + * classifier failure was indistinguishable from a normal skip. + * The workflow's `if ! ... ; then skip` idiom would silently drop + * a commit on tool failure. + * + * 2. Classifier missing at the base tag: the workflow runs + * `git checkout -b "$BRANCH" "$BASE_TAG"` BEFORE the cherry-pick + * loop, which replaces the working tree with the base tag's + * contents. Base tags predating #2980 (notably v1.39.0, the most + * likely next hotfix base) don't have + * `scripts/diff-touches-shipped-paths.cjs` at all. `node ` + * exits non-zero → workflow treats as "not shipped" → every + * commit gets silently dropped → empty hotfix branch published. + * This is strictly worse than the original #2980 push-rejection, + * which at least failed loudly. + * + * Fix: + * - Script: distinct exit codes (0 = shipped, 1 = not shipped, + * 2 = classifier error). All uncaught failure paths + * (uncaughtException, unhandledRejection, fs/JSON errors) route + * to exit 2. + * - Workflow: stage the classifier into $RUNNER_TEMP at the top of + * `Prepare hotfix branch` (before `git checkout -b "$BASE_TAG"`) + * and reference $CLASSIFIER in the loop. Capture exit code via + * ${PIPESTATUS[1]} and dispatch via case: 0 → proceed, 1 → skip + * (NON_SHIPPED_SKIPPED), anything else → ::error:: + exit. The + * workflow refuses to start if the classifier source is missing + * in the dispatched ref. + */ + +'use strict'; + +// allow-test-rule: source-text-is-the-product +// release-sdk.yml IS the product for hotfix automation; the static +// assertions extract the "Prepare hotfix branch" run block via +// indentation-aware YAML parsing rather than raw-text grep across the +// whole document. + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); + +const REPO_ROOT = path.join(__dirname, '..'); +const WORKFLOW_PATH = path.join(REPO_ROOT, '.github', 'workflows', 'release-sdk.yml'); +const CLASSIFIER_PATH = path.join(REPO_ROOT, 'scripts', 'diff-touches-shipped-paths.cjs'); + +function extractStepRun(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; + let j = i + 1; + while (j < lines.length) { + const peek = lines[j]; + if (/^\s*- /.test(peek)) { + const peekIndent = peek.match(/^(\s*)/)[1].length; + if (peekIndent <= stepIndent) break; + } + const runMatch = peek.match(/^(\s*)run:\s*\|(?:[+-])?\s*$/); + if (runMatch) { + const blockIndent = runMatch[1].length + 2; + const body = []; + for (let k = j + 1; k < lines.length; k++) { + const bodyLine = lines[k]; + if (bodyLine.length === 0) { + body.push(''); + continue; + } + const lead = bodyLine.match(/^(\s*)/)[1].length; + if (lead < blockIndent && bodyLine.trim() !== '') break; + body.push(bodyLine.slice(blockIndent)); + } + return body.join('\n'); + } + j++; + } + throw new Error(`step "${stepName}" found but no run: | block before step end`); + } + throw new Error(`step "${stepName}" not found in workflow`); +} + +describe('bug-2983: shipped-paths classifier exit-code discipline', () => { + function runClassifier({ stdin, cwd }) { + return spawnSync('node', [CLASSIFIER_PATH], { + cwd, + input: stdin, + encoding: 'utf8', + }); + } + + function makeFixtureRepo({ files, raw }) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-2983-')); + if (raw !== undefined) { + fs.writeFileSync(path.join(tmp, 'package.json'), raw); + } else if (files !== undefined) { + fs.writeFileSync( + path.join(tmp, 'package.json'), + JSON.stringify({ name: 'fixture', version: '0.0.0', files }, null, 2) + ); + } + return tmp; + } + + test('exit 0 still means "at least one shipped path"', () => { + const tmp = makeFixtureRepo({ files: ['bin'] }); + try { + const r = runClassifier({ stdin: 'bin/foo.js\n', cwd: tmp }); + assert.equal(r.status, 0, `expected exit 0 for shipped path; stderr=${r.stderr}`); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('exit 1 still means "no shipped paths" — preserved across the #2983 refactor', () => { + const tmp = makeFixtureRepo({ files: ['bin'] }); + try { + const r = runClassifier({ stdin: 'tests/foo.test.cjs\n.github/workflows/release-sdk.yml\n', cwd: tmp }); + assert.equal(r.status, 1, `expected exit 1 for non-shipping diff; stderr=${r.stderr}`); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('exit 2 when package.json is missing — distinguishable from "not shipped"', () => { + // Run in a temp dir with no package.json. Pre-#2983 this would + // surface as exit 1 (Node default for uncaught throw), which the + // workflow would have silently treated as "not shipped." Post-fix + // it's exit 2, which the workflow MUST treat as a hard error. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-2983-no-pkg-')); + try { + const r = runClassifier({ stdin: 'bin/foo.js\n', cwd: tmp }); + assert.equal(r.status, 2, `expected exit 2 for missing package.json; got ${r.status}; stderr=${r.stderr}`); + assert.match(r.stderr, /diff-touches-shipped-paths/, 'classifier error must be tagged in stderr so the workflow operator can find it'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('exit 2 when package.json is malformed JSON', () => { + const tmp = makeFixtureRepo({ raw: '{ this is not json' }); + try { + const r = runClassifier({ stdin: 'bin/foo.js\n', cwd: tmp }); + assert.equal(r.status, 2, `expected exit 2 for malformed package.json; got ${r.status}; stderr=${r.stderr}`); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('module exports the exit-code constants so workflow tests can reference them by name', () => { + // Decoupling intent (EXIT_NOT_SHIPPED) from value (1) is what makes + // a future "let's renumber" edit safe. Importers should reference + // the constants, not the literals. + const mod = require(CLASSIFIER_PATH); + assert.equal(mod.EXIT_SHIPPED, 0, 'EXIT_SHIPPED must be 0'); + assert.equal(mod.EXIT_NOT_SHIPPED, 1, 'EXIT_NOT_SHIPPED must be 1'); + assert.equal(mod.EXIT_ERROR, 2, 'EXIT_ERROR must be 2 (distinct from 0 and 1)'); + }); +}); + +describe('bug-2983: workflow stages the classifier and dispatches on exit code', () => { + test('classifier is staged into $RUNNER_TEMP before any working-tree-mutating git command', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + // The staging cp must appear before `git checkout -b ... "$BASE_TAG"` + // — that's the operation that overwrites the working tree with the + // base tag's contents, which may not contain the classifier. + const stageIdx = script.search(/cp "\$CLASSIFIER_SRC" "\$CLASSIFIER"/); + const checkoutIdx = script.search(/git checkout -b "\$BRANCH" "\$BASE_TAG"/); + assert.ok( + stageIdx !== -1, + 'workflow must `cp` the classifier from its in-tree path to a stable location ($CLASSIFIER) before the working tree is swapped (#2983)' + ); + assert.ok( + checkoutIdx !== -1, + 'workflow must contain the base-tag checkout — sentinel that establishes the staging-must-precede ordering constraint' + ); + assert.ok( + stageIdx < checkoutIdx, + `classifier staging must precede \`git checkout -b ... "$BASE_TAG"\` so the source file isn't already gone (#2983). Found stage at offset ${stageIdx}, checkout at ${checkoutIdx}.` + ); + }); + + test('staging targets $RUNNER_TEMP — survives the working-tree swap and is auto-cleaned by the runner', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + assert.match( + script, + /CLASSIFIER="\$\{RUNNER_TEMP\}\/diff-touches-shipped-paths\.cjs"/, + '$CLASSIFIER must point at $RUNNER_TEMP — that path survives `git checkout` (lives outside the repo) and is cleaned automatically by the runner (#2983)' + ); + }); + + test('workflow refuses to run if the classifier source is missing in the dispatched ref', () => { + // Defense in depth: if a future edit reorders the steps so the + // first checkout doesn't put the classifier on disk, the workflow + // must fail loudly rather than skip every commit. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + assert.match( + script, + /if \[ ! -f "\$CLASSIFIER_SRC" \][\s\S]{0,200}::error::shipped-paths classifier not found/, + 'workflow must fail-fast if scripts/diff-touches-shipped-paths.cjs is missing in the dispatched ref (#2983)' + ); + assert.match( + script, + /if \[ ! -f "\$CLASSIFIER" \][\s\S]{0,200}failed to stage classifier/, + 'workflow must fail-fast if cp didn\'t actually produce $CLASSIFIER (defense against silent cp failure on RUNNER_TEMP corner cases) (#2983)' + ); + }); + + test('cherry-pick loop captures classifier exit code via $PIPESTATUS and dispatches on the value', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + // The pre-#2983 form was `if ! ... | node ...; then skip; fi` which + // collapses every non-zero exit (including missing-script and + // uncaught-throw cases) into the skip path. The required new shape + // is: run the pipeline, snapshot $PIPESTATUS into a local array + // immediately, dispatch via case. + // + // CodeRabbit on PR #2984 caught a subtler bug in the first iteration + // of this fix: `pipeline || true; RC=${PIPESTATUS[1]}` doesn't work + // because `|| true` runs `true` as a one-command pipeline when the + // pipeline fails (exit 1 or 2 — exactly the cases we care about), + // overwriting PIPESTATUS to (0). The hardened form snapshots + // PIPESTATUS into a local array on the line immediately after the + // pipeline, with no intervening commands. + assert.match( + script, + /PIPE_RC=\("\$\{PIPESTATUS\[@\]\}"\)/, + 'cherry-pick loop must snapshot the entire $PIPESTATUS array via `PIPE_RC=("${PIPESTATUS[@]}")` immediately after the classifier pipeline — `${PIPESTATUS[1]}` direct-read is unsafe under any subsequent simple command, and `|| true; ${PIPESTATUS[1]}` is broken because `|| true` runs `true` as its own pipeline on the failure paths (CodeRabbit on PR #2984)' + ); + // The pipeline must run under `set +e` to allow the snapshot — at + // the workflow's top-level `set -euo pipefail`, a non-zero exit + // from the pipeline would otherwise terminate the step before the + // snapshot line runs. + assert.match( + script, + /set \+e[\s\S]{0,200}node "\$CLASSIFIER"[\s\S]{0,80}PIPE_RC=\("\$\{PIPESTATUS\[@\]\}"\)[\s\S]{0,40}set -e/, + 'classifier pipeline must be wrapped `set +e` ... pipeline ... `PIPE_RC=("${PIPESTATUS[@]}")` ... `set -e` — any other shape either misses the snapshot or terminates the step early (#2983, CodeRabbit on PR #2984)' + ); + // Must NOT use the broken `pipeline || true; RC=${PIPESTATUS[1]}` form. + // The `|| true` rewrites PIPESTATUS on the failure paths. + assert.doesNotMatch( + script, + /node "\$CLASSIFIER"\s*\|\|\s*true\s*\n\s*CLASSIFIER_RC="\$\{PIPESTATUS\[1\]\}"/, + 'classifier pipeline must NOT use `|| true` followed by `${PIPESTATUS[1]}` — `|| true` runs `true` as a one-command pipeline on the failure paths and overwrites PIPESTATUS to (0), so PIPESTATUS[1] becomes unset (CodeRabbit on PR #2984)' + ); + // Must NOT use the original `if ! ... | node ...; then` shape either. + assert.doesNotMatch( + script, + /if ! git diff-tree[\s\S]{0,200}node[\s\S]{0,200}\.cjs[^|\n]*; then/, + 'cherry-pick loop must NOT use `if ! ... | node classifier; then skip` — that shape silently treats classifier errors as skips (#2983)' + ); + // The case dispatch must explicitly handle 0, 1, and a default branch. + assert.match( + script, + /case "\$CLASSIFIER_RC" in[\s\S]+?0\)[\s\S]+?1\)[\s\S]+?\*\)/, + 'case dispatch on $CLASSIFIER_RC must list 0, 1, and a default-error branch in that order so each is handled explicitly (#2983)' + ); + }); + + test('git diff-tree failure is also fail-fast (not silently classified as not-shipped)', () => { + // The new array-snapshot form gives us $DIFFTREE_RC for free. + // git diff-tree is unlikely to fail on a known-good $SHA, but if + // it does (e.g., $SHA is corrupt or fetch was incomplete), we must + // not pipe partial/empty output into the classifier and call it + // "not shipped." Fail-fast with ::error:: instead. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + assert.match( + script, + /DIFFTREE_RC="\$\{PIPE_RC\[0\]\}"/, + 'workflow must extract git diff-tree\'s exit from PIPE_RC[0] so a partial-pipeline failure can be distinguished from a clean classifier result (CodeRabbit on PR #2984)' + ); + assert.match( + script, + /if \[ "\$DIFFTREE_RC" -ne 0 \][\s\S]{0,200}::error::git diff-tree failed/, + 'workflow must emit ::error:: and exit when git diff-tree itself fails — silently passing partial input to the classifier would defeat the whole point of #2983 (CodeRabbit on PR #2984)' + ); + }); + + test('hotfix run summary no longer falsely advertises a merge-back PR', () => { + // CodeRabbit on PR #2984: the Summary block still printed + // "Merge-back PR opened against main" even though the merge-back + // step was removed. Operators reading the summary would expect a PR + // that was never opened. Replace with explicit non-action text so + // the summary accurately describes what happened. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + + assert.doesNotMatch( + yaml, + /echo "- Merge-back PR opened against main"/, + 'run summary must NOT advertise a merge-back PR — the step was removed in #2983 and the line is stale (CodeRabbit on PR #2984)' + ); + assert.match( + yaml, + /No merge-back PR \(auto-picked commits are already on main\)/, + 'run summary must explicitly state that no merge-back PR exists, with the rationale, so operators understand it\'s intentional rather than missing (CodeRabbit on PR #2984)' + ); + }); + + test('default-error branch fails the workflow with ::error:: rather than continuing', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + // Must emit ::error:: AND exit non-zero. Either alone is + // insufficient: ::error:: without exit just decorates the log; + // exit without ::error:: hides the cause. + assert.match( + script, + /\*\)[\s\S]+?::error::shipped-paths classifier failed[\s\S]+?exit "\$CLASSIFIER_RC"/, + 'classifier-error branch must emit ::error:: AND `exit "$CLASSIFIER_RC"` — silently continuing would defeat the whole point of #2983 (#2983)' + ); + }); + + test('merge-back PR step is removed (auto-cherry-pick hotfix has nothing to merge back)', () => { + // Auto-cherry-pick only picks commits already on main, so by + // construction every code change on the hotfix branch is already + // there. The only hotfix-branch-only commit is `chore: bump version + // ... for hotfix`, which either no-ops or rewinds main's + // in-progress version. The merge-back step was vestigial and was + // additionally blocked by org policy ("GitHub Actions is not + // permitted to create or approve pull requests"). Run 25232968975 + // was the trigger. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + assert.doesNotMatch( + yaml, + /Open merge-back PR \(hotfix only\)/, + 'merge-back PR step must be removed — nothing to merge back when every commit is already on main (#2983)' + ); + assert.doesNotMatch( + yaml, + /chore: merge hotfix v\$\{VERSION\} back to main/, + 'merge-back PR title must be gone — vestigial from a different release flow (#2983)' + ); + // Job-level pull-requests permission was granted solely for the + // merge-back step. Removing the step means revoking the permission + // (least-privilege). + assert.doesNotMatch( + yaml, + /pull-requests: write/, + 'release job must NOT request `pull-requests: write` after the merge-back removal — least-privilege requires dropping the unused scope (#2983)' + ); + }); + + test('the staged path is what the loop invokes, not the in-tree path', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + // Find the cherry-pick loop's classifier invocation and ensure it + // references "$CLASSIFIER", not scripts/diff-touches-shipped-paths.cjs + // directly. Allowing the in-tree path here would re-introduce the + // base-tag-missing bug. + const loopAnchor = script.indexOf('CANDIDATES=$(git cherry HEAD origin/main'); + assert.ok(loopAnchor !== -1, 'cherry-pick loop sentinel not found'); + // 8 KB window matching the bug-2964 test's bound (raised from 6 KB + // when the PIPESTATUS-snapshot hardening on PR for #2984's CR + // findings pushed the cherry-pick call further past the loop anchor). + const window = script.slice(loopAnchor, loopAnchor + 8000); + assert.match( + window, + /node "\$CLASSIFIER"/, + 'cherry-pick loop must invoke `node "$CLASSIFIER"` (the staged copy), not `node scripts/diff-touches-shipped-paths.cjs` (the in-tree path) — the in-tree path may have been replaced by `git checkout -b "$BASE_TAG"` (#2983)' + ); + assert.doesNotMatch( + window, + /node scripts\/diff-touches-shipped-paths\.cjs/, + 'cherry-pick loop must NOT invoke `node scripts/diff-touches-shipped-paths.cjs` — base tags predating #2980 don\'t have that file in their tree (#2983)' + ); + }); +});