From 608be0e7cf523d687f701bd6a6063761a25b3284 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 1 Aug 2026 10:21:27 -0400 Subject: [PATCH] fix(#2913): distinguish empty cherry-pick from genuine conflict in hotfix create (#2970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2913): add regression for hotfix empty-cherry-pick discrimination Two layers: (1) real-git test proving the discrimination logic (check for unmerged paths → skip if empty, abort if conflict) is correct; (2) source-text assertions proving the logic and summary heading are in release.yml. Before the fix: release.yml treats any non-zero cherry-pick exit as a conflict, so an already-applied commit (empty pick) aborts the entire hotfix create. * fix(#2913): distinguish empty cherry-pick from genuine conflict git cherry-pick exits non-zero for BOTH genuine conflicts AND empty picks (the change is already present by content). The hotfix create job treated every non-zero exit as a conflict, so an already-applied commit (notably the structural 'chore: sync next package version' that follows every release finalize) aborted the entire run. Now the error handler checks for unmerged paths (git diff --diff-filter=U): - No unmerged paths → already applied by content → skip, record, continue. - Unmerged paths present → genuine conflict → existing abort/push/exit-1 behavior and operator guidance, unchanged. The job summary now has a separate 'Skipped (already applied by content)' heading, distinct from 'Skipped (feat/refactor/etc)'. * fix(#2913): address review — post-skip continuation test + conflict observability Two findings from isolated adversarial review: 1. MINOR (test gap): no test proved the sequencer is clean after --skip, so a regression switching --skip to --quit would pass green. Added a second cherry-pick after the skip asserting it succeeds. 2. MINOR (observability): SKIPPED_EMPTY was dropped on the conflict-exit path — already-applied commits before a genuine conflict were silently lost from the summary. Now the conflict summary emits them under a dedicated heading. * chore(#2913): add changeset fragment * chore(#2913): backfill changeset PR number 2970 --------- Co-authored-by: sim --- .changeset/eager-voles-frolic.md | 5 + .github/workflows/release.yml | 35 +++- .../release-hotfix-empty-cherry-pick.test.cjs | 197 ++++++++++++++++++ 3 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 .changeset/eager-voles-frolic.md create mode 100644 tests/release-hotfix-empty-cherry-pick.test.cjs diff --git a/.changeset/eager-voles-frolic.md b/.changeset/eager-voles-frolic.md new file mode 100644 index 000000000..5490526fc --- /dev/null +++ b/.changeset/eager-voles-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2970 +--- +**Hotfix branches with auto cherry-pick no longer abort on already-applied commits** — cutting a hotfix from a tag whose `chore: sync next package version` commit applied empty (already present by content) aborted the entire create run. The cherry-pick error handler now distinguishes empty picks (no unmerged paths → skip) from genuine conflicts (unmerged paths → abort), and the job summary lists skipped-as-empty commits separately. (#2913) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4c72c2635..a90efe9bb 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -216,6 +216,7 @@ jobs: INCLUDED="" SKIPPED="" + SKIPPED_EMPTY="" while IFS= read -r SHA; do [ -z "$SHA" ] && continue SUBJECT=$(git log -1 --format='%s' "$SHA") @@ -223,9 +224,26 @@ jobs: if echo "$SUBJECT" | grep -qE '^(fix|chore)(\([^)]+\))?!?: '; then echo "→ cherry-picking $SHA $SUBJECT" if ! git cherry-pick -x "$SHA"; then - # Abort restores HEAD to the last successful pick. On real - # runs, push that state so the operator can fetch, resolve - # $SHA manually, and finalize with auto_cherry_pick=false. + # #2913: git cherry-pick exits non-zero for BOTH genuine conflicts + # AND empty picks (the change is already present by content). + # Distinguish them: a genuine conflict has unmerged paths; an + # empty pick has none. `git cherry` compares patch-ids against a + # different base, so a commit already present by content can still + # be marked `+` — and applying it produces an empty diff. + UNMERGED=$(git diff --name-only --diff-filter=U) + if [ -z "$UNMERGED" ]; then + # Already applied by content — skip and continue the loop. + # This is the structural case: every release finalize produces + # a `chore: sync next package version` commit that is a no-op + # against the tag the next hotfix branches from. + git cherry-pick --skip || true + echo " skip $SHA $SUBJECT (already applied by content — empty pick)" + SKIPPED_EMPTY="${SKIPPED_EMPTY}- \`${SHA}\` ${SUBJECT}"$'\n' + continue + fi + # Genuine conflict — abort restores HEAD to the last successful + # pick. On real runs, push that state so the operator can fetch, + # resolve $SHA manually, and finalize with auto_cherry_pick=false. git cherry-pick --abort || true if [ "$DRY_RUN" != "true" ]; then git push --force-with-lease origin "$BRANCH" || git push origin "$BRANCH" || true @@ -235,6 +253,12 @@ jobs: echo "" echo "Failed at: \`${SHA}\` — \`${SUBJECT}\`" echo "" + if [ -n "$SKIPPED_EMPTY" ]; then + echo "### Skipped before this conflict (already applied by content — empty pick)" + echo "" + echo "$SKIPPED_EMPTY" + echo "" + fi if [ "$DRY_RUN" = "true" ]; then echo "**Dry run:** branch was not pushed, so the picks below were discarded with the runner." if [ -n "$INCLUDED" ]; then @@ -274,6 +298,11 @@ jobs: echo "_No fix/chore commits to include._" echo "" fi + if [ -n "$SKIPPED_EMPTY" ]; then + echo "### Skipped (already applied by content — empty pick)" + echo "" + echo "$SKIPPED_EMPTY" + fi if [ -n "$SKIPPED" ]; then echo "### Skipped (feat/refactor/etc — not auto-included)" echo "" diff --git a/tests/release-hotfix-empty-cherry-pick.test.cjs b/tests/release-hotfix-empty-cherry-pick.test.cjs new file mode 100644 index 000000000..1f9d8e0a0 --- /dev/null +++ b/tests/release-hotfix-empty-cherry-pick.test.cjs @@ -0,0 +1,197 @@ +// allow-test-rule: source-text-is-the-product (see #2913) +// .github/workflows/release.yml is the deployed CI contract; the cherry-pick +// error handler is bash inside the YAML. This test has two layers: +// 1. Real-git: proves the DISCRIMINATION LOGIC (check for unmerged paths → +// skip if empty, abort if conflict) is correct by exercising it against +// a real git repo with both scenarios. +// 2. Source-text: asserts the discrimination logic AND the separate summary +// heading actually made it into release.yml — so a regression that +// removes the check from the workflow is caught even if the logic test +// still passes. + +'use strict'; + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); +const { spawnSync } = require('node:child_process'); +const { cleanup } = require('./helpers.cjs'); + +const RELEASE_WORKFLOW = path.join(__dirname, '..', '.github', 'workflows', 'release.yml'); + +// ─── git helpers ──────────────────────────────────────────────────────────── + +function git(cwd, ...args) { + const r = spawnSync('git', ['-C', cwd, ...args], { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] }); + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')} failed (status ${r.status}) in ${cwd}:\n${r.stderr || r.stdout}`); + } + return r.stdout.trim(); +} + +function makeRepo(dir) { + fs.mkdirSync(dir, { recursive: true }); + spawnSync('git', ['init', dir], { encoding: 'utf8' }); + git(dir, 'config', 'user.email', 'test@test'); + git(dir, 'config', 'user.name', 'Test'); + git(dir, 'config', 'commit.gpgsign', 'false'); + // Ensure the default branch is 'main' regardless of the system's + // init.defaultBranch setting (older Git defaults to 'master'). + spawnSync('git', ['-C', dir, 'symbolic-ref', 'HEAD', 'refs/heads/main'], { encoding: 'utf8' }); + return dir; +} + +function commitFile(dir, file, content, message) { + fs.writeFileSync(path.join(dir, file), content); + git(dir, 'add', file); + git(dir, 'commit', '-m', message); + return git(dir, 'rev-parse', 'HEAD'); +} + +/** + * The discrimination logic extracted from release.yml's cherry-pick error + * handler. This mirrors what the bash does after `git cherry-pick -x "$SHA"` + * exits non-zero: + * - Check for unmerged paths (`git diff --name-only --diff-filter=U`). + * - Empty → skip (already applied by content), record, continue. + * - Non-empty → genuine conflict, abort, return conflict info. + * + * @returns {{ status: 'empty' | 'conflict', unmerged: string[] }} + */ +function discriminateCherryPick(dir) { + const unmerged = git(dir, 'diff', '--name-only', '--diff-filter=U'); + const unmergedFiles = unmerged.split('\n').filter(Boolean); + if (unmergedFiles.length === 0) { + git(dir, 'cherry-pick', '--skip'); + return { status: 'empty', unmerged: [] }; + } + git(dir, 'cherry-pick', '--abort'); + return { status: 'conflict', unmerged: unmergedFiles }; +} + +// ─── real-git tests: prove the discrimination logic ───────────────────────── + +describe('#2913 — cherry-pick empty-vs-conflict discrimination (real git)', () => { + let tmpDir; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2913-')); + }); + + after(() => { + cleanup(tmpDir); + }); + + test('an already-applied-by-content commit is detected as empty and skipped', () => { + const repo = makeRepo(path.join(tmpDir, 'empty-pick')); + // Seed: create file.txt with "v1" + commitFile(repo, 'file.txt', 'v1\n', 'init'); + const baseSha = git(repo, 'rev-parse', 'HEAD'); + + // On "next": bump file.txt to v2, then create a chore commit that sets it back to v1 then to v2 + git(repo, 'checkout', '-b', 'next'); + commitFile(repo, 'file.txt', 'v2\n', 'feat: bump to v2'); + // Now create a commit that is a no-op against base by content: set file.txt to "v1\n" + // then immediately set it back — but that creates a non-empty diff. Instead, + // simulate the exact scenario: a commit whose PATCH applies empty because + // the content is already present. + // + // Create the "chore" commit on a throwaway branch from base, then cherry-pick + // it onto next where the change is already present. + git(repo, 'checkout', baseSha); + git(repo, 'checkout', '-b', 'throwaway'); + // Apply a change that sets file.txt to v2 (same content as next already has) + const choreSha = commitFile(repo, 'file.txt', 'v2\n', 'chore: sync version to v2'); + + // Go back to next and try to cherry-pick the chore commit. + // file.txt is already v2 on next → the patch applies empty. + git(repo, 'checkout', 'next'); + + // Attempt the cherry-pick — it exits non-zero (empty). + const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', choreSha], { encoding: 'utf8' }); + assert.notStrictEqual(r.status, 0, 'cherry-pick of an already-applied commit must exit non-zero'); + + // Apply the discrimination logic. + const result = discriminateCherryPick(repo); + assert.strictEqual(result.status, 'empty', 'an empty pick must be classified as "empty", not "conflict"'); + assert.deepStrictEqual(result.unmerged, []); + + // After skip, HEAD should be back on next (no cherry-pick state). + const headAfter = git(repo, 'rev-parse', 'next'); + const currentHead = git(repo, 'rev-parse', 'HEAD'); + assert.strictEqual(currentHead, headAfter, 'after skip, HEAD should be unchanged (no new commit applied)'); + + // #2913 MINOR 1: after --skip, the sequencer must be clean so the next + // iteration's cherry-pick succeeds. A regression that switched --skip to + // --quit (which leaves sequencer state) would fail here. + git(repo, 'checkout', baseSha); + git(repo, 'checkout', '-b', 'throwaway2'); + const realSha = commitFile(repo, 'other.txt', 'real change\n', 'fix: a real fix'); + git(repo, 'checkout', 'next'); + const realPick = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', realSha], { encoding: 'utf8' }); + assert.strictEqual(realPick.status, 0, + `after --skip, the sequencer must be clean so the next cherry-pick succeeds; got status ${realPick.status}:\n${realPick.stderr || realPick.stdout}`); + }); + + test('a genuine cherry-pick conflict is detected as conflict and aborted', () => { + const repo = makeRepo(path.join(tmpDir, 'conflict-pick')); + // Seed: file.txt with "base line" + commitFile(repo, 'file.txt', 'base line\n', 'init'); + + // On "next": change line 1 to "next version" + git(repo, 'checkout', '-b', 'next'); + const conflictingSha = commitFile(repo, 'file.txt', 'next version\n', 'fix: change to next version'); + + // Go back to main and make a DIFFERENT change to the same line. + git(repo, 'checkout', 'main'); + commitFile(repo, 'file.txt', 'main version\n', 'fix: change to main version'); + + // Attempt to cherry-pick the next commit — it conflicts (same line, different content). + const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', conflictingSha], { encoding: 'utf8' }); + assert.notStrictEqual(r.status, 0, 'cherry-pick of a conflicting commit must exit non-zero'); + + // Apply the discrimination logic. + const result = discriminateCherryPick(repo); + assert.strictEqual(result.status, 'conflict', 'a genuine conflict must be classified as "conflict"'); + assert.ok(result.unmerged.length > 0, 'a genuine conflict must have unmerged files'); + assert.ok(result.unmerged.includes('file.txt'), 'file.txt must be in the unmerged list'); + }); +}); + +// ─── source-text assertions: prove the logic is in release.yml ────────────── + +describe('#2913 — release.yml cherry-pick error handler discriminates empty from conflict', () => { + const text = fs.existsSync(RELEASE_WORKFLOW) + ? fs.readFileSync(RELEASE_WORKFLOW, 'utf8') + : ''; + + test('the cherry-pick error handler checks for unmerged paths before aborting', () => { + // The fix adds a `git diff --name-only --diff-filter=U` check inside the + // `if ! git cherry-pick` block. Without it, every empty pick aborts the run. + assert.ok(text.length > 0, 'release.yml must exist'); + assert.ok( + text.includes('--diff-filter=U'), + 'release.yml cherry-pick error handler must check for unmerged paths (git diff --diff-filter=U) before aborting — without this, an already-applied commit aborts the entire hotfix create (#2913)', + ); + }); + + test('empty picks are skipped (git cherry-pick --skip), not aborted', () => { + assert.ok(text.length > 0, 'release.yml must exist'); + assert.ok( + text.includes('cherry-pick --skip'), + 'release.yml must skip (not abort) already-applied cherry-picks — git cherry-pick --skip continues the loop (#2913)', + ); + }); + + test('the job summary distinguishes already-applied from not-fix-chore skips', () => { + assert.ok(text.length > 0, 'release.yml must exist'); + // The fix introduces a separate variable/heading for already-applied commits + // so they are not silently dropped or confused with not-fix/chore skips. + assert.ok( + /already applied|applied by content|SKIPPED_EMPTY|empty pick/i.test(text), + 'release.yml job summary must distinguish "skipped (already applied)" from "skipped (not fix/chore)" — a separate heading or variable for empty picks (#2913)', + ); + }); +});