* fix(#2983): classifier exit-code discipline, base-tag staging, drop vestigial merge-back Three issues surfaced by CodeRabbit's post-merge review of #2981 plus a production failure on the v1.39.1 release run. (1) Overloaded classifier exit code scripts/diff-touches-shipped-paths.cjs reused exit 1 for both the legitimate "no shipped paths" result and Node's default exit on uncaught throw, so any classifier failure (corrupt package.json, EPERM, etc.) was indistinguishable from a normal skip — the workflow's `if ! ... ; then skip` idiom would silently drop the commit. Distinct exit codes now: 0 shipped — at least one path is in the npm `files` whitelist 1 not shipped — CI / test / docs / planning only 2 classifier error — workflow MUST fail-fast uncaughtException + unhandledRejection + try/catch around fs/JSON parsing all route to exit 2 with stderr context. (2) Classifier missing at the base tag (CRITICAL) `Prepare hotfix branch` runs `git checkout -b "$BRANCH" "$BASE_TAG"` BEFORE the cherry-pick loop, replacing 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 <missing>` exits non-zero — `if !` skips every commit — empty hotfix branch published. Strictly worse than the original #2980 push-rejection, which at least failed loudly. Stage the classifier from the dispatched ref's working tree into $RUNNER_TEMP at the top of the run script (before any working-tree- mutating git command). The cherry-pick loop now references $CLASSIFIER (staged) instead of the in-tree path. Sanity guards: refuse to start if scripts/diff-touches-shipped-paths.cjs is missing in the dispatched ref, refuse to proceed if cp didn't materialize $CLASSIFIER. The cherry-pick loop captures node's exit via ${PIPESTATUS[1]} and dispatches via explicit case: 0 proceed with cherry-pick 1 skip into NON_SHIPPED_SKIPPED * emit ::error:: + exit "$CLASSIFIER_RC" (3) Drop the merge-back PR step Auto-cherry-pick only picks commits already on main (`git cherry HEAD origin/main` outputs the unmerged ones; we filter 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 either no-ops against main or rewinds main's in-progress version. The merge-back PR was vestigial. It also failed in production on run 25232968975 with `GitHub Actions is not permitted to create or approve pull requests (createPullRequest)` — org policy blocks PR creation from the workflow's GH_TOKEN. Even without that block, the PR would have nothing useful to merge. Step removed. The `pull-requests: write` permission granted solely for the merge-back step has been dropped from the release job (least-privilege). Regression coverage tests/bug-2983-classifier-exit-codes-and-base-tag-staging.test.cjs adds 12 assertions across two describe blocks: - 5 classifier behavioral: exit 0/1 preserved, exit 2 on missing package.json, exit 2 on malformed JSON, exit-code constants exported. - 7 workflow contract: classifier staged before checkout, target is $RUNNER_TEMP, missing-source guard, missing-staged guard, PIPESTATUS-based dispatch, error branch fails workflow, loop uses staged path (not in-tree). tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs updated where it asserted the pre-#2983 `if ! ... ; then` shape: now accepts the post-#2983 case-dispatch form. The test still proves the classifier participates; bug-2983 enforces the specific shape. Run summary references for the curious reviewer: - Run 25232010071 — original #2980 trigger (workflow-file push rejection) - Run 25232968975 — failed merge-back step that prompted the "is this even useful?" question that drove the removal Closes #2983 * fix(#2983): address CodeRabbit findings on PR #2984 Two findings, both real, both fixed. (1) [Critical] PIPESTATUS capture clobbered by `|| true` Pre-fix shape: git diff-tree ... | node "$CLASSIFIER" || true CLASSIFIER_RC="${PIPESTATUS[1]}" When the classifier exits 1 ("not shipped" — common case) or 2 (error), `|| true` triggers the right-hand side. `true` is a one-command "pipeline" that overwrites PIPESTATUS to (0). ${PIPESTATUS[1]} on the next line is therefore unset (or stale under set -u). The case dispatch then matched the empty string — falling into `*)` and failing the workflow on every non-shipped commit, OR matching `0)` after some shells default-init unset to 0 and silently picking commits that don't ship. Local repro confirms the issue: $ bash -c 'set -euo pipefail; false | sh -c "exit 7" || true; \ echo "PIPESTATUS: ${PIPESTATUS[*]}"; \ echo "[1]: ${PIPESTATUS[1]:-<unset>}"' PIPESTATUS: 0 [1]: <unset> Fix: bracket the pipeline in `set +e`/`set -e`, snapshot PIPESTATUS into a local array on the very next line, then dispatch on the snapshot: set +e git diff-tree ... | node "$CLASSIFIER" PIPE_RC=("${PIPESTATUS[@]}") set -e DIFFTREE_RC="${PIPE_RC[0]}" CLASSIFIER_RC="${PIPE_RC[1]}" The snapshot must happen on the first line after the pipeline; any intervening simple command resets PIPESTATUS. The array form is invariant against that. Bonus from the new shape: $DIFFTREE_RC is now also captured. git diff-tree is unlikely to fail on a known-good $SHA, but if it does, we no longer feed partial/empty input to the classifier and call it "not shipped." A non-zero DIFFTREE_RC emits ::error::git diff-tree failed and exits. (2) [Minor] Stale "Merge-back PR opened against main" summary line The hotfix run summary still printed: echo "- Merge-back PR opened against main" But the merge-back step itself was removed in the previous commit on this branch. Operators reading the summary would expect a PR that doesn't exist. Replaced with explicit non-action text: echo "- No merge-back PR (auto-picked commits are already on main)" Test coverage bug-2983 test file gains 3 assertions: - PIPE_RC array-snapshot pattern is required (regex matches the exact `PIPE_RC=("${PIPESTATUS[@]}")` form). - The `pipeline || true; ${PIPESTATUS[1]}` antipattern is explicitly forbidden via assert.doesNotMatch. - DIFFTREE_RC is captured from PIPE_RC[0] and a non-zero value triggers ::error::git diff-tree failed. - Run summary forbids `Merge-back PR opened against main` and requires the new non-action sentence. bug-2964 test's loop-anchor window bumped 6 KB → 8 KB to accommodate the additional pre-pick scaffolding (the test's own comment had already anticipated this kind of growth, citing prior precedents from #2970 and #2980). Mark CodeRabbit comments resolved post-commit. Refs CR finding ids 3175253571, 3175253578 on PR #2984.
145 lines
7.2 KiB
JavaScript
145 lines
7.2 KiB
JavaScript
/**
|
|
* Regression test for bug #2964
|
|
*
|
|
* The release-sdk hotfix workflow's auto_cherry_pick loop aborted the entire
|
|
* run if any commit between the base tag and origin/main had an empty diff
|
|
* against its parent (e.g. a squash-merge whose contents were already merged
|
|
* via an earlier PR). `git cherry-pick -x` exits non-zero on empty commits
|
|
* with "The previous cherry-pick is now empty", and the workflow's loop
|
|
* (`if ! git cherry-pick -x "$SHA"; then ... exit 1`) treated any non-zero
|
|
* as a hard conflict — bricking every hotfix the moment a no-op commit
|
|
* landed on main.
|
|
*
|
|
* Fix: pass `--allow-empty --keep-redundant-commits` so empty picks are
|
|
* preserved on the hotfix branch (with `-x` provenance, matching main 1:1)
|
|
* and picks whose diff resolves to empty after applying to the new base
|
|
* also pass cleanly. Real conflicts still surface — the flags only change
|
|
* the empty-commit exit code.
|
|
*
|
|
* This test asserts both:
|
|
* 1. Static — the workflow YAML carries the flags on the cherry-pick call
|
|
* inside the auto_cherry_pick loop. If a future edit drops them, this
|
|
* regresses immediately.
|
|
* 2. Behavioral — `git cherry-pick -x --allow-empty --keep-redundant-commits`
|
|
* against a real empty commit in a throwaway repo exits 0 (proves the
|
|
* flags semantically do what we claim), while plain `git cherry-pick -x`
|
|
* exits non-zero against the same commit (proves the bug exists without
|
|
* the flags).
|
|
*/
|
|
|
|
'use strict';
|
|
|
|
// allow-test-rule: source-text-is-the-product
|
|
// The release-sdk.yml workflow IS the product for hotfix automation —
|
|
// GitHub Actions executes the YAML's shell verbatim. Testing the text
|
|
// content tests the deployed contract: if the flags are absent, the
|
|
// empty-commit guarantee is absent.
|
|
|
|
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 WORKFLOW_PATH = path.join(__dirname, '..', '.github', 'workflows', 'release-sdk.yml');
|
|
|
|
function git(cwd, args, env = {}) {
|
|
// Force-disable signing inline so a developer's global gpgsign / sshsign
|
|
// config can't fail commits in this throwaway repo. Don't rely on env
|
|
// because gpg.format/user.signingkey live in gitconfig, not env vars.
|
|
const signingOff = ['-c', 'commit.gpgsign=false', '-c', 'tag.gpgsign=false', '-c', 'gpg.format=openpgp', '-c', 'user.signingkey='];
|
|
return spawnSync('git', [...signingOff, ...args], {
|
|
cwd,
|
|
encoding: 'utf8',
|
|
env: { ...process.env, ...env, GIT_AUTHOR_NAME: 'test', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 'test', GIT_COMMITTER_EMAIL: 't@t' },
|
|
});
|
|
}
|
|
|
|
describe('bug-2964: release-sdk hotfix cherry-pick survives empty commits', () => {
|
|
test('release-sdk.yml passes --allow-empty --keep-redundant-commits in the auto_cherry_pick loop', () => {
|
|
const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8');
|
|
|
|
// Find the auto_cherry_pick block by anchoring on a line unique to it,
|
|
// then assert the cherry-pick invocation inside that block carries both
|
|
// flags. We deliberately scope to the loop — a stray `git cherry-pick`
|
|
// elsewhere in the file (none today) would not satisfy this contract.
|
|
const loopAnchor = yaml.indexOf('CANDIDATES=$(git cherry HEAD origin/main');
|
|
assert.ok(
|
|
loopAnchor !== -1,
|
|
'release-sdk.yml must contain the auto_cherry_pick loop that derives candidates via `git cherry HEAD origin/main` (#2964)'
|
|
);
|
|
|
|
// 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,
|
|
// 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 + 8000);
|
|
const pickMatch = /git\b[^\n]*?cherry-pick[^\n]*"\$SHA"/.exec(window);
|
|
assert.ok(
|
|
pickMatch,
|
|
'auto_cherry_pick loop must invoke `git ... cherry-pick ... "$SHA"` (#2964)'
|
|
);
|
|
|
|
const pickLine = pickMatch[0];
|
|
assert.ok(
|
|
pickLine.includes('--allow-empty'),
|
|
`auto_cherry_pick must pass --allow-empty so empty no-op commits on main do not abort the hotfix (#2964). Found: ${pickLine}`
|
|
);
|
|
assert.ok(
|
|
pickLine.includes('--keep-redundant-commits'),
|
|
`auto_cherry_pick must pass --keep-redundant-commits so commits whose diff resolves to empty after rebasing onto the base tag do not abort the hotfix (#2964). Found: ${pickLine}`
|
|
);
|
|
});
|
|
|
|
test('git cherry-pick with --allow-empty --keep-redundant-commits succeeds on an empty commit; without them it fails', () => {
|
|
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-2964-'));
|
|
try {
|
|
// Build a synthetic repo with one real commit on main and one truly
|
|
// empty commit on top — same shape as the real upstream artifact
|
|
// (b328f326 on origin/main has tree == its parent's tree).
|
|
assert.equal(git(tmp, ['init', '-q', '-b', 'main']).status, 0, 'git init');
|
|
fs.writeFileSync(path.join(tmp, 'README.md'), 'base\n');
|
|
assert.equal(git(tmp, ['add', 'README.md']).status, 0, 'git add');
|
|
assert.equal(git(tmp, ['commit', '-q', '-m', 'base']).status, 0, 'base commit');
|
|
assert.equal(git(tmp, ['tag', 'v0.0.0']).status, 0, 'tag base');
|
|
// Make a genuinely empty commit on main.
|
|
assert.equal(git(tmp, ['commit', '--allow-empty', '-q', '-m', 'fix: noop on main']).status, 0, 'empty commit');
|
|
const empty = git(tmp, ['rev-parse', 'HEAD']).stdout.trim();
|
|
assert.ok(empty.length === 40, `expected sha, got ${empty}`);
|
|
|
|
// Reset to the base tag (simulates the hotfix branch starting from v0.0.0).
|
|
assert.equal(git(tmp, ['checkout', '-q', '-b', 'hotfix/0.0.1', 'v0.0.0']).status, 0, 'checkout hotfix');
|
|
|
|
// Without the flags: cherry-pick of an empty commit fails.
|
|
const without = git(tmp, ['cherry-pick', '-x', empty]);
|
|
assert.notEqual(
|
|
without.status,
|
|
0,
|
|
'plain `git cherry-pick -x` MUST fail on an empty commit — if this passes, git semantics changed and the bug premise is gone (#2964)'
|
|
);
|
|
// Reset cherry-pick state for the next run.
|
|
git(tmp, ['cherry-pick', '--abort']);
|
|
// git may have already auto-resolved to a clean state; ensure we're back to v0.0.0.
|
|
git(tmp, ['reset', '--hard', 'v0.0.0']);
|
|
|
|
// With the flags (matching what the workflow now uses): success.
|
|
const withFlags = git(tmp, ['cherry-pick', '-x', '--allow-empty', '--keep-redundant-commits', empty]);
|
|
assert.equal(
|
|
withFlags.status,
|
|
0,
|
|
`git cherry-pick -x --allow-empty --keep-redundant-commits MUST succeed on an empty commit (#2964). stderr: ${withFlags.stderr}`
|
|
);
|
|
} finally {
|
|
fs.rmSync(tmp, { recursive: true, force: true });
|
|
}
|
|
});
|
|
});
|