fix(release-sdk): skip cherry-picks whose target context is absent at base (#2967)

* fix(release-sdk): skip cherry-picks whose target context is absent at base

When auto_cherry_pick processed a fix:/chore: commit whose patch modified
code that didn't exist at the hotfix base tag — typically because the
surrounding infrastructure was added later in a feat/refactor commit
excluded by the filter — `git cherry-pick` failed with a conflict that
no operator could meaningfully resolve, and the loop bricked the run.

Discovered re-running the 1.39.1 dry-run after #2965 merged: cherry-pick
of `a3467792` (the #2965 merge itself) failed because the auto_cherry_pick
block it modifies was added in #2956 ("Add automated cherry-pick + SDK-
bundle parity to hotfix flow") — an Add/feat commit, so the fix/chore
filter excludes it. v1.39.0 has no such block, so the patch had no
anchor.

The conflict is unmistakably distinguishable from a real content conflict:
git emits marker blocks where every `<<<<<<< HEAD ... =======` HEAD
section is empty (no anchor lines to reconcile against), while real
conflicts have content on both sides.

After cherry-pick fails:
  1. List unmerged paths via `git diff --diff-filter=U`.
  2. For each, scan conflict markers with awk. If every HEAD section is
     blank/whitespace-only across every block, classify as
     context-missing.
  3. Context-missing → `git cherry-pick --skip` and append to SKIPPED
     list with reason "(context absent at base)".
  4. Otherwise fall through to the existing abort/push-partial/error
     path that surfaces the conflict for operator resolution.

Real conflicts still surface with the same workflow as before.

Tests in tests/bug-2966-cherry-pick-context-missing.test.cjs cover:
  - Static — extracts the "Prepare hotfix branch" run block via
    indentation-aware YAML parsing (no raw-text grep) and asserts the
    classification predicate, --skip call, and skipped-reason annotation
    are present.
  - Behavioral — synthetic repo reproducing the real shape of the
    failure, asserts cherry-pick exits non-zero and produces the
    empty-HEAD marker shape.
  - Predicate — pulls the awk script out of the deployed workflow and
    feeds it sample conflict shapes (empty-HEAD, real, mixed,
    whitespace-only); asserts each is classified as the workflow will
    behave.

Local: `node --test tests/bug-2966-...test.cjs` → 3/3 pass.
Local: `npm run lint:tests` → 0 violations.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* fix(release-sdk): pin merge.conflictStyle=merge on hotfix cherry-pick

CodeRabbit flagged on #2967 that the awk classifier introduced for #2966
assumes default conflict-marker style (plain `<<<<<<< HEAD ... ======= ...
>>>>>>>`). If a runner has merge.conflictStyle=diff3 or zdiff3 set
(globally, repo-config, or via git defaults shift), the marker emits an
extra `||||||| ancestor` section between HEAD and =======. The awk's
`in_head` mode would accumulate that ancestor content into the HEAD
buffer, and a context-missing conflict would misclassify as real —
sending the workflow into the abort path on a pick that should be
silently skipped.

Pass `-c merge.conflictStyle=merge` on the cherry-pick command itself
(scoped to that one git invocation; doesn't leak to other commands).
This guarantees marker shape regardless of the runner's git config.

Updated the existing static assertion in
tests/bug-2966-cherry-pick-context-missing.test.cjs to require the pin —
a future edit dropping it fails the test.

Local: `node --test tests/bug-2966-...test.cjs` → 3/3 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* test(#2964): allow git options between `git` and `cherry-pick`

The previous commit on this branch (d6530190) added
`git -c merge.conflictStyle=merge cherry-pick ...` to release-sdk.yml.
The bug-2964 static test's regex `/git cherry-pick[^\n]*"\$SHA"/`
required `cherry-pick` to be the literal next token after `git`, so it
no longer matched the line and CI failed on Node 22 / Node 24 / macOS.

Loosen to `/git\b[^\n]*?cherry-pick[^\n]*"\$SHA"/` so any options
between `git` and `cherry-pick` (e.g. `-c key=value`) are tolerated.
The flag assertions on the matched line still verify --allow-empty and
--keep-redundant-commits are present, which is what bug-2964 actually
guards.

Local: `node --test tests/bug-2964-...test.cjs tests/bug-2966-...test.cjs`
→ 5/5 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

* test(#2966): pin merge.conflictStyle in test git wrapper, assert awk status

CodeRabbit raised two issues on PR #2967:

1. The synthetic-repo cherry-pick reproducer asserted `<<<<<<< HEAD ...`
   blocks have empty HEAD sections, but the cherry-pick itself didn't
   pin `merge.conflictStyle`. A developer or CI runner with global
   diff3/zdiff3 config would inject `||||||| ancestor` lines into the
   HEAD scan and the test would fail for environment reasons rather
   than the bug premise. Pin the style on the test's `git()` wrapper
   so every git operation in the test is deterministic regardless of
   user config.

2. `classify()` ran awk and consumed `r.stdout.trim()` without checking
   `r.status` or `r.error`. A failed awk invocation (missing binary,
   syntax error, signal) returns empty stdout, which would falsely
   classify as "context-missing" and the test would silently pass on
   broken predicates. Add `assert.ok(!r.error, ...)` and
   `assert.equal(r.status, 0, ...)` before reading stdout.

Local: `node --test tests/bug-2966-...test.cjs tests/bug-2964-...test.cjs`
→ 5/5 pass.

https://claude.ai/code/session_01LApueb9PVs2uSBhsLprVzG

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
Tom Boucher
2026-05-01 14:35:18 -04:00
committed by GitHub
parent a346779213
commit 0d25ef0c47
3 changed files with 340 additions and 3 deletions

View File

@@ -167,7 +167,52 @@ jobs:
SUBJECT=$(git log -1 --format='%s' "$SHA")
if echo "$SUBJECT" | grep -qE '^(fix|chore)(\([^)]+\))?!?: '; then
echo "→ cherry-picking $SHA $SUBJECT"
if ! git cherry-pick -x --allow-empty --keep-redundant-commits "$SHA"; then
# Pin merge.conflictStyle=merge on the cherry-pick so the
# awk classifier below sees deterministic marker shapes —
# diff3/zdiff3 would inject `||||||| ancestor` lines into
# the HEAD section and cause context-missing conflicts to
# misclassify as real. Bug #2966.
if ! git -c merge.conflictStyle=merge cherry-pick -x --allow-empty --keep-redundant-commits "$SHA"; then
# Distinguish patch-context-missing (the picked commit
# modifies code that doesn't exist at the base — typically
# because the surrounding infrastructure was added in a
# feat/refactor commit excluded by this filter) from a real
# content conflict (operator must resolve). Signal: a
# context-missing conflict produces marker blocks where the
# `<<<<<<< HEAD ... =======` HEAD section is empty for every
# block in every conflicted file. A real conflict has
# non-blank content in the HEAD section. Marker style is
# pinned on the cherry-pick command above. Bug #2966.
UNMERGED=$(git diff --name-only --diff-filter=U)
CONTEXT_MISSING_ONLY=true
if [ -z "$UNMERGED" ]; then
CONTEXT_MISSING_ONLY=false
fi
while IFS= read -r CONFLICTED; do
[ -z "$CONFLICTED" ] && continue
REAL=$(awk '
/^<<<<<<< / { in_head=1; head=""; next }
/^=======$/ && in_head { in_head=0; next }
/^>>>>>>> / {
if (head ~ /[^[:space:]]/) { print "real"; exit }
head=""
next
}
in_head { head = head $0 "\n" }
' "$CONFLICTED")
if [ "$REAL" = "real" ]; then
CONTEXT_MISSING_ONLY=false
break
fi
done <<< "$UNMERGED"
if [ "$CONTEXT_MISSING_ONLY" = "true" ]; then
echo "↷ skipping $SHA — patch context absent at $BASE_TAG"
git cherry-pick --skip
SKIPPED="${SKIPPED}- \`${SHA}\` ${SUBJECT} (context absent at base)"$'\n'
continue
fi
git cherry-pick --abort || true
# On real runs: push the partial-pick state so the operator
# can fetch + resolve $SHA + push + re-run with auto_cherry_pick=false.

View File

@@ -72,11 +72,14 @@ describe('bug-2964: release-sdk hotfix cherry-pick survives empty commits', () =
// The cherry-pick call lives within ~30 lines of the anchor. Limit the
// window to avoid matching unrelated cherry-pick references elsewhere.
// 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 + 2000);
const pickMatch = /git cherry-pick[^\n]*"\$SHA"/.exec(window);
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)'
'auto_cherry_pick loop must invoke `git ... cherry-pick ... "$SHA"` (#2964)'
);
const pickLine = pickMatch[0];

View File

@@ -0,0 +1,289 @@
/**
* Regression test for bug #2966
*
* The release-sdk hotfix workflow's auto_cherry_pick loop aborts when a
* `fix:`/`chore:` commit's patch is rooted in code that doesn't exist at
* the hotfix's base tag (e.g. the surrounding block was added later in a
* feat/refactor commit excluded by the filter). The conflict is
* unresolvable — the patch literally cannot be applied to a tree that
* lacks the surrounding infrastructure — but the workflow treats it as
* an operator-resolvable conflict and exits.
*
* Fix: after `git cherry-pick` exits non-zero, inspect each unmerged
* file's conflict markers. If every conflict block in every file has an
* empty `<<<<<<< HEAD ... =======` HEAD section, run `git cherry-pick
* --skip` and add the SHA to the skipped list with reason
* "context absent at base". Else, fall through to the existing abort/
* push-partial/error path.
*
* This test asserts both:
* 1. Static — the auto_cherry_pick loop in release-sdk.yml carries the
* context-missing detection (matching `git cherry-pick --skip` and
* `context absent at base` semantics) so the no-source-grep static
* check is still meaningful for future edits.
* 2. Behavioral — using a synthetic git repo that reproduces the exact
* shape of the failure on origin/main:
* a. A patch whose target context doesn't exist at base produces
* empty-HEAD conflict markers AND a non-zero exit from
* cherry-pick. (Proves the bug premise.)
* b. The `awk` predicate in the workflow correctly classifies the
* empty-HEAD case as "context-missing" (skippable) and the
* both-sides-have-content case as "real" (must abort).
*/
'use strict';
// allow-test-rule: source-text-is-the-product
// release-sdk.yml IS the product for hotfix automation; GitHub Actions
// executes the YAML's shell verbatim. The static check uses structured
// extraction (extractStepRun) rather than raw-text grep, scoped to the
// "Prepare hotfix branch" step's run block.
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');
/**
* Extract the `run:` literal block of a named step from a GitHub Actions
* workflow using indentation-aware parsing — no raw-text grep across the
* whole document. Walks lines once, recognises `- name:` step headers and
* `run: |` literal-block markers, and returns the unindented script body.
*
* No YAML library is used; the repo has none in dependencies and adding
* one for a single test isn't justified.
*/
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`);
}
function git(cwd, args) {
// Force-disable signing inline — a developer's global gpgsign config
// can't be allowed to fail commits in this throwaway repo. Also pin
// merge.conflictStyle=merge so the cherry-pick reproducer below sees
// the same marker shape the workflow guards against (diff3/zdiff3 in
// the developer or CI runner's global config would inject `|||||||`
// sections and break the empty-HEAD assertion).
const inlineConfig = [
'-c', 'commit.gpgsign=false',
'-c', 'tag.gpgsign=false',
'-c', 'gpg.format=openpgp',
'-c', 'user.signingkey=',
'-c', 'merge.conflictStyle=merge',
];
return spawnSync('git', [...inlineConfig, ...args], {
cwd,
encoding: 'utf8',
env: { ...process.env, GIT_AUTHOR_NAME: 'test', GIT_AUTHOR_EMAIL: 't@t', GIT_COMMITTER_NAME: 'test', GIT_COMMITTER_EMAIL: 't@t' },
});
}
describe('bug-2966: release-sdk hotfix cherry-pick distinguishes context-missing from real conflicts', () => {
test('Prepare hotfix branch step skips on context-missing conflicts and aborts on real ones', () => {
const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const script = extractStepRun(yaml, 'Prepare hotfix branch');
// The loop must detect unmerged paths after a failed cherry-pick.
assert.match(
script,
/git diff --name-only --diff-filter=U/,
'auto_cherry_pick must read the unmerged path list after a failed cherry-pick to classify the conflict (#2966)'
);
// The empty-HEAD-section detector must be present.
assert.match(
script,
/<<<<<<< /,
'auto_cherry_pick must inspect conflict markers to classify context-missing vs real conflicts (#2966)'
);
// The skip path must call `git cherry-pick --skip` so the loop continues
// past commits whose target context doesn't exist at the base tag.
assert.match(
script,
/git cherry-pick --skip/,
'auto_cherry_pick must invoke `git cherry-pick --skip` for context-missing conflicts so they don\'t brick the run (#2966)'
);
// The skipped list must annotate the reason so operators see it in the
// run summary (not silently disappear).
assert.match(
script,
/context absent at base/,
'auto_cherry_pick must annotate skipped picks with "context absent at base" so the run summary surfaces them (#2966)'
);
// The cherry-pick must pin merge.conflictStyle=merge so the awk
// classifier sees deterministic marker shapes regardless of the
// runner's git config (diff3/zdiff3 would inject `||||||| ancestor`
// lines into the HEAD section and misclassify context-missing
// conflicts as real ones).
assert.match(
script,
/git -c merge\.conflictStyle=merge cherry-pick/,
'auto_cherry_pick must pin `merge.conflictStyle=merge` on the cherry-pick command so marker parsing is deterministic across runner git configs (#2966)'
);
});
test('cherry-pick of a patch whose target section is absent at base produces empty-HEAD conflict markers and exits non-zero', () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-2966-ctx-missing-'));
try {
assert.equal(git(tmp, ['init', '-q', '-b', 'main']).status, 0, 'git init');
// Base — file exists but does NOT contain the section the patch will modify.
fs.mkdirSync(path.join(tmp, '.github', 'workflows'), { recursive: true });
fs.writeFileSync(path.join(tmp, '.github/workflows/x.yml'), 'name: base\njobs:\n release:\n runs-on: ubuntu-latest\n');
assert.equal(git(tmp, ['add', '.']).status, 0, 'git add base');
assert.equal(git(tmp, ['commit', '-q', '-m', 'base']).status, 0, 'commit base');
assert.equal(git(tmp, ['tag', 'v0.0.0']).status, 0, 'tag base');
// feat (excluded by fix/chore filter) — adds the prepare block.
fs.writeFileSync(path.join(tmp, '.github/workflows/x.yml'),
'name: base\njobs:\n prepare:\n run: |\n git cherry-pick -x "$SHA"\n release:\n runs-on: ubuntu-latest\n');
assert.equal(git(tmp, ['commit', '-qam', 'feat: add prepare block']).status, 0, 'commit feat');
// fix — modifies the line inside the prepare block.
const yaml = fs.readFileSync(path.join(tmp, '.github/workflows/x.yml'), 'utf8')
.replace('git cherry-pick -x "$SHA"', 'git cherry-pick -x --allow-empty "$SHA"');
fs.writeFileSync(path.join(tmp, '.github/workflows/x.yml'), yaml);
assert.equal(git(tmp, ['commit', '-qam', 'fix: tweak cherry-pick']).status, 0, 'commit fix');
const fixSha = git(tmp, ['rev-parse', 'HEAD']).stdout.trim();
// Cherry-pick fix onto v0.0.0 — must conflict because target context isn't there.
assert.equal(git(tmp, ['checkout', '-q', '-b', 'hotfix', 'v0.0.0']).status, 0, 'checkout hotfix');
const pick = git(tmp, ['cherry-pick', '-x', '--allow-empty', '--keep-redundant-commits', fixSha]);
assert.notEqual(
pick.status,
0,
'cherry-pick of a patch whose target section is absent at base MUST exit non-zero (the bug premise: workflow currently treats this as a real conflict and aborts) (#2966)'
);
// Confirm conflict markers exist and the HEAD section is empty in every block.
const conflicted = fs.readFileSync(path.join(tmp, '.github/workflows/x.yml'), 'utf8');
assert.match(conflicted, /<<<<<<< /, 'conflict markers must be written to the file');
// Every <<<<<<< HEAD ... ======= block must have empty HEAD content.
const blocks = [];
let inHead = false;
let head = '';
for (const line of conflicted.split('\n')) {
if (/^<<<<<<< /.test(line)) { inHead = true; head = ''; continue; }
if (/^=======$/.test(line) && inHead) { inHead = false; continue; }
if (/^>>>>>>> /.test(line)) { blocks.push(head); head = ''; continue; }
if (inHead) head += line + '\n';
}
assert.ok(blocks.length > 0, 'expected at least one conflict marker block');
for (const b of blocks) {
assert.equal(b.trim(), '', `expected every HEAD section to be empty (context-missing signal), got: ${JSON.stringify(b)}`);
}
} finally {
fs.rmSync(tmp, { recursive: true, force: true });
}
});
test('the awk predicate from the workflow classifies empty-HEAD as skippable and non-empty-HEAD as real', () => {
// Pull the awk script out of the deployed workflow so this test
// exercises the exact predicate that runs in CI — not a copy.
const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8');
const script = extractStepRun(yaml, 'Prepare hotfix branch');
const awkMatch = script.match(/awk '\n([\s\S]+?)' "\$CONFLICTED"/);
assert.ok(awkMatch, 'expected to find the conflict-classifying awk script in the workflow');
const awkProgram = awkMatch[1];
function classify(conflictText) {
const tmpFile = path.join(os.tmpdir(), `bug-2966-awk-${process.pid}-${Date.now()}-${Math.random()}.txt`);
fs.writeFileSync(tmpFile, conflictText);
try {
const r = spawnSync('awk', [awkProgram, tmpFile], { encoding: 'utf8' });
// Fail loudly on awk execution errors — silently consuming an
// empty stdout from a crashed/missing awk would let context-missing
// assertions falsely pass.
assert.ok(!r.error, `awk failed to launch: ${r.error && r.error.message}`);
assert.equal(r.status, 0, `awk predicate exited non-zero: ${r.stderr || '(no stderr)'}`);
return r.stdout.trim();
} finally {
fs.rmSync(tmpFile, { force: true });
}
}
// Empty HEAD section → context-missing → no "real" emitted.
const ctxMissing = [
'unrelated line',
'<<<<<<< HEAD',
'=======',
'patch wants this content',
'and this',
'>>>>>>> sha (msg)',
'tail',
].join('\n');
assert.equal(classify(ctxMissing), '', 'awk must classify empty-HEAD blocks as context-missing (no "real" emitted) (#2966)');
// Non-empty HEAD section → real conflict.
const realConflict = [
'<<<<<<< HEAD',
'VALUE=existing',
'=======',
'VALUE=patched',
'>>>>>>> sha (msg)',
].join('\n');
assert.equal(classify(realConflict), 'real', 'awk must classify non-empty-HEAD blocks as real conflicts (#2966)');
// Mixed — first block empty-HEAD, second block real → real wins (overall classification).
const mixed = [
'<<<<<<< HEAD',
'=======',
'patch content',
'>>>>>>> sha (msg)',
'spacer',
'<<<<<<< HEAD',
'something existing',
'=======',
'something patched',
'>>>>>>> sha (msg)',
].join('\n');
assert.equal(classify(mixed), 'real', 'awk must report "real" if any conflict block has non-empty HEAD content (#2966)');
// Whitespace-only HEAD section → context-missing (the awk predicate
// treats blank/whitespace HEAD content the same as empty).
const whitespaceHead = [
'<<<<<<< HEAD',
' ',
'\t',
'=======',
'patch content',
'>>>>>>> sha (msg)',
].join('\n');
assert.equal(classify(whitespaceHead), '', 'awk must classify whitespace-only HEAD blocks as context-missing (#2966)');
});
});