From 0efe4f06125618c616856fab189ca39c83f80f61 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 15 May 2026 22:43:21 -0400 Subject: [PATCH] fix(3621): release-sdk hotfix cherry-picks test-fixture commits (#3623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(3621): cherry-pick test-fixture commits in hotfix runs The release-sdk hotfix loop excluded test-fixture updates that align CI with a cherry-picked production fix, leaving the hotfix branch with new production behavior and stale test assertions. Broke v1.42.3 CI (run 25949422676) when fix(3562) was cherry-picked but its bundled test correction in docs(3562) commit 08848df8 was POLICY_SKIPPED by the prefix filter. Two-part fix: 1. release-sdk.yml prefix regex now accepts test: alongside fix:/chore:. feat:, docs:, refactor: still POLICY_SKIPPED as before. 2. scripts/diff-touches-shipped-paths.cjs treats tests/-rooted paths and sdk/src vitest specs as CI-gating-equivalent. A test: commit touching only those paths now passes the shipped-paths gate. The #2980 push-blocking guard is preserved as a separate first-priority check: any commit touching .github/workflows/ still skips regardless of test paths in the same bundle, because the default GITHUB_TOKEN lacks the workflow scope and the push step would fail. New regression coverage in tests/bug-3621-cherry-pick-test-fixtures.test.cjs: - workflow prefix regex includes test: - isCiGating accepts tests/ and sdk/src vitest specs, rejects non-spec sdk/src paths and incidental "test" name occurrences - classifier exits 0 for test-only, mixed test+docs, and the original shipped paths - classifier exits 1 for pure docs-only and workflow-only diffs - new explicit assertion that #2980 push-blocking wins over #3621: workflow + test + changelog bundle still skips Adjusted one pre-existing bug-2980 test fixture to use a non- push-blocking non-shipped path (planning/notes.md) instead of .github/workflows/release-sdk.yml. The original assertion was documenting "mixed diff includes shipped path → include" but its fixture happened to also trigger the push-blocking guard now made explicit by this PR. Fixes #3621 Co-Authored-By: Claude Opus 4.7 (1M context) * fix(release-sdk): align hotfix summary labels with test matcher * fix(3621): align operator-facing strings with the fix/chore/test matcher The candidate-loop regex was updated to accept test: but several human-facing strings in the same job still read fix/chore. Update every description/comment/summary line for consistency so operators reading the run summary or workflow_dispatch inputs see the same set of accepted prefixes the matcher actually applies. Also corrected the NON_SHIPPED_SKIPPED summary text — it claimed test changes belong on main, not in a hotfix. That assumption is what #3621 fixes; tests under tests/ and sdk/src vitest specs are now CI-gating candidates and may be picked. The summary now scopes the never-pick guidance to CI / docs / planning paths only. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/3621-cherry-pick-test-fixtures.md | 5 + .github/workflows/release-sdk.yml | 20 +- scripts/diff-touches-shipped-paths.cjs | 52 ++++- ...otfix-only-picks-shipping-changes.test.cjs | 11 +- ...ug-3621-cherry-pick-test-fixtures.test.cjs | 217 ++++++++++++++++++ 5 files changed, 288 insertions(+), 17 deletions(-) create mode 100644 .changeset/3621-cherry-pick-test-fixtures.md create mode 100644 tests/bug-3621-cherry-pick-test-fixtures.test.cjs diff --git a/.changeset/3621-cherry-pick-test-fixtures.md b/.changeset/3621-cherry-pick-test-fixtures.md new file mode 100644 index 000000000..84d6ebdee --- /dev/null +++ b/.changeset/3621-cherry-pick-test-fixtures.md @@ -0,0 +1,5 @@ +--- +"get-shit-done-cc": patch +--- + +**Fixed: hotfix releases now correctly cherry-pick test-fixture updates.** The `release-sdk.yml` auto-cherry-pick loop accepts `test:` commits (in addition to `fix:` and `chore:`), and the shipped-paths classifier treats `tests/` and `sdk/src//.test.` as CI-gating-equivalent. When a production fix is bundled with a `fix:` commit but the matching test-fixture alignment lands in a separate `test:` commit, both are now picked together — preventing the CI-red state that broke the v1.42.3 hotfix attempt. The `.github/workflows/` push-blocking guard is preserved (#2980): bundles touching workflow files still skip regardless of other content. diff --git a/.github/workflows/release-sdk.yml b/.github/workflows/release-sdk.yml index ba0cc0fd1..4395eecef 100644 --- a/.github/workflows/release-sdk.yml +++ b/.github/workflows/release-sdk.yml @@ -26,7 +26,7 @@ on: workflow_dispatch: inputs: action: - description: 'publish = normal dev/next/latest publish; hotfix = create hotfix/X.YY.Z branch from latest vX.YY.* tag, cherry-pick fix:/chore: from main, publish to @latest' + description: 'publish = normal dev/next/latest publish; hotfix = create hotfix/X.YY.Z branch from latest vX.YY.* tag, cherry-pick fix:/chore:/test: from main, publish to @latest' required: true type: choice default: publish @@ -51,7 +51,7 @@ on: required: false type: string auto_cherry_pick: - description: 'Hotfix only: auto-cherry-pick fix:/chore: commits from origin/main since base tag.' + description: 'Hotfix only: auto-cherry-pick fix:/chore:/test: commits from origin/main since base tag.' required: false type: boolean default: true @@ -74,7 +74,7 @@ jobs: # # action=publish → outputs inputs.ref verbatim (may be empty = workflow ref) # action=hotfix → branches hotfix/X.YY.Z from highest existing vX.YY.* tag, - # auto-cherry-picks fix:/chore: from origin/main, pushes, + # auto-cherry-picks fix:/chore:/test: from origin/main, pushes, # and outputs the new branch as ref. Idempotent: if branch # already exists (operator pre-prepared it via hotfix.yml), # we just check it out and re-run the cherry-pick step @@ -179,10 +179,10 @@ jobs: | grep -F -f <(echo "$CANDIDATES") || true) INCLUDED="" # POLICY_SKIPPED — commits intentionally not picked because they - # don't match the fix/chore filter (feat/refactor/docs/etc). - # CONFLICT_SKIPPED — fix/chore commits whose cherry-pick failed + # don't match the fix/chore/test filter (feat/refactor/docs/etc). + # CONFLICT_SKIPPED — fix/chore/test commits whose cherry-pick failed # and were skipped per the full-automation policy (#2968). - # NON_SHIPPED_SKIPPED — fix/chore commits whose diff doesn't + # NON_SHIPPED_SKIPPED — fix/chore/test commits whose diff doesn't # touch any path in the npm tarball's `files` whitelist # (CI / test / docs / planning-only changes). They can't # affect the published package's behavior, so picking them @@ -199,7 +199,7 @@ jobs: while IFS= read -r SHA; do [ -z "$SHA" ] && continue SUBJECT=$(git log -1 --format='%s' "$SHA") - if echo "$SUBJECT" | grep -qE '^(fix|chore)(\([^)]+\))?!?: '; then + if echo "$SUBJECT" | grep -qE '^(fix|chore|test)(\([^)]+\))?!?: '; then # Merge commits with fix:/chore: titles can't be cherry-picked # without `-m ` and we can't pick the parent # automatically. They fail BEFORE entering cherry-pick state @@ -358,16 +358,16 @@ jobs: echo "Base: \`$BASE_TAG\` → Branch: \`$BRANCH\`$([ "$DRY_RUN" = "true" ] && echo " (DRY RUN — local only)")" echo "" if [ -n "$INCLUDED" ]; then - echo "### Included (fix/chore)" + echo "### Included (fix/chore/test)" echo "" echo "$INCLUDED" else - echo "_No fix/chore commits to include._" + echo "_No fix/chore/test commits to include._" fi if [ -n "$NON_SHIPPED_SKIPPED" ]; then echo "### Skipped — touches no shipped paths (informational)" echo "" - echo "These fix/chore commits don't touch any path in the npm tarball's \`files\` whitelist (or \`package.json\`), so they cannot change the published package's behavior. CI / test / docs / planning-only changes belong on \`main\`, not in a hotfix. No action needed." + echo "These fix/chore/test commits don't touch any path the hotfix CI cares about — neither the npm tarball's \`files\` whitelist (or \`package.json\`) nor a CI-gating test under \`tests/\` or \`sdk/src/**\` vitest specs (#3621). CI / docs / planning-only changes belong on \`main\`, not in a hotfix. No action needed." echo "" echo "$NON_SHIPPED_SKIPPED" fi diff --git a/scripts/diff-touches-shipped-paths.cjs b/scripts/diff-touches-shipped-paths.cjs index f91855885..33e771fac 100644 --- a/scripts/diff-touches-shipped-paths.cjs +++ b/scripts/diff-touches-shipped-paths.cjs @@ -12,6 +12,12 @@ * - package.json (always included by `npm pack`, regardless of `files`) * - every entry in package.json `files`, treated as either an exact * file match or a directory prefix (matching `npm pack` semantics). + * - CI-gating test paths: `tests/` plus + * `sdk/src//.test.` and `.spec.` variants + * — these don't ship in the tarball, but they gate the hotfix-branch + * test job. A test fixture update that aligns with a cherry-picked + * production fix MUST be pickable or CI fails on the hotfix run. + * #3621 — root cause of the v1.42.3 hotfix red CI. * * `package-lock.json` is intentionally NOT considered shipped — `npm pack` * excludes it from the tarball unless it's explicitly in `files`, and at @@ -48,6 +54,24 @@ function loadShipPrefixes(pkgPath) { return ['package.json', ...files]; } +// #3621: paths that gate hotfix-branch CI even though they don't appear +// in the npm tarball. When a cherry-picked production fix changes behavior +// that an existing test on the v1.42.2 base asserts against, the matching +// test fixture from `main` must also be cherry-picked or CI fails on the +// hotfix run (exactly what happened on v1.42.3 — production fix(3562) was +// picked, the bundled test-fixture correction in commit 08848df8 was not). +// Combined with the `test:` prefix being added to the candidate-loop regex +// in release-sdk.yml, this lets `test(####):` fixture-alignment commits be +// cherry-picked alongside their production counterparts. +function isCiGating(diffPath) { + if (diffPath.startsWith('tests/')) return true; + // SDK vitest specs live next to source. Production source ships via + // sdk/dist/ (already in package.json `files`); the test files are what's + // missing from that surface. + if (diffPath.startsWith('sdk/src/') && /\.(test|spec)\.(ts|cjs|mjs|js)$/.test(diffPath)) return true; + return false; +} + function isShipped(diffPath, shipPrefixes) { // Normalize Windows-style separators just in case (git always emits // forward slashes, but a developer running this locally on a different @@ -56,6 +80,14 @@ function isShipped(diffPath, shipPrefixes) { return shipPrefixes.some((s) => p === s || p.startsWith(s + '/')); } +// #2980: commits that touch `.github/workflows/*` cannot be cherry-picked +// onto a hotfix branch because the default GITHUB_TOKEN lacks the +// `workflow` permission and the push step fails. Detect them upfront so a +// `test:`-eligible commit bundling a workflow edit still gets skipped. +function isPushBlocking(diffPath) { + return diffPath.replace(/\\/g, '/').startsWith('.github/workflows/'); +} + function fail(message, err) { process.stderr.write(`diff-touches-shipped-paths: ${message}\n`); if (err && err.stack) process.stderr.write(`${err.stack}\n`); @@ -86,8 +118,22 @@ function main() { process.stdin.on('end', () => { 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); + // #2980 still wins over #3621: any commit touching .github/workflows/* + // is unpickable regardless of other content because the push step + // fails on workflow scope rejection. Check this first. + if (paths.some(isPushBlocking)) { + process.exit(EXIT_NOT_SHIPPED); + } + if (paths.some((p) => isShipped(p, shipPrefixes))) { + process.exit(EXIT_SHIPPED); + } + // #3621: a commit whose only relevant paths are CI-gating tests is + // still pickable — it can change whether the hotfix CI passes even + // though it doesn't change what the npm tarball ships. + if (paths.some(isCiGating)) { + process.exit(EXIT_SHIPPED); + } + process.exit(EXIT_NOT_SHIPPED); } catch (err) { fail('classification failed', err); } @@ -98,4 +144,4 @@ if (require.main === module) { main(); } -module.exports = { loadShipPrefixes, isShipped, EXIT_SHIPPED, EXIT_NOT_SHIPPED, EXIT_ERROR }; +module.exports = { loadShipPrefixes, isShipped, isCiGating, isPushBlocking, EXIT_SHIPPED, EXIT_NOT_SHIPPED, EXIT_ERROR }; 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 1e0da0a43..02685ab50 100644 --- a/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs +++ b/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs @@ -265,12 +265,15 @@ describe('bug-2980: scripts/diff-touches-shipped-paths.cjs classifier semantics' }); test('mixed diff is shipped if ANY path is shipped', () => { - // A commit that touches both a shipped file and a non-shipped file - // must be classified as shipped — the non-shipped paths are along - // for the ride, but the commit can still affect what ships. + // A commit that touches both a shipped file and an unrelated non- + // shipped, non-push-blocking file must be classified as shipped — the + // non-shipped paths are along for the ride, but the commit can still + // affect what ships. Note: this fixture deliberately avoids + // `.github/workflows/*`, which is push-blocking (#3621) and forces a + // skip regardless of other shipped paths in the same bundle. const tmp = makeFixtureRepo(['bin']); try { - const stdin = '.github/workflows/release-sdk.yml\nbin/foo.js\ntests/bar.test.cjs\n'; + const stdin = 'CHANGELOG.md\nbin/foo.js\nplanning/notes.md\n'; assert.equal(runClassifier(stdin, tmp).status, 0, 'mixed diff with at least one shipped path must classify as shipped'); } finally { fs.rmSync(tmp, { recursive: true, force: true }); diff --git a/tests/bug-3621-cherry-pick-test-fixtures.test.cjs b/tests/bug-3621-cherry-pick-test-fixtures.test.cjs new file mode 100644 index 000000000..573a1b792 --- /dev/null +++ b/tests/bug-3621-cherry-pick-test-fixtures.test.cjs @@ -0,0 +1,217 @@ +/** + * Regression test for bug #3621 + * + * The release-sdk hotfix cherry-pick loop's `fix:`/`chore:` prefix filter + * and the shipped-paths classifier together excluded a test-fixture update + * that was required to align CI with a cherry-picked production fix. + * + * Concrete failure shape (v1.42.3 hotfix, run 25949422676): + * - Commit 36534059 fix(3562) was picked. It changed the installer to + * materialize gsd-named SKILL.md files under the codex skills dir. + * - Commit 08848df8 (docs(3562) prefix) was POLICY_SKIPPED by the prefix + * filter. It bundled the matching test-fixture correction (removing an + * "if (runtime === 'codex') return new Set()" short-circuit in + * tests/install-minimal-all-runtimes.test.cjs). + * - Result: hotfix branch has new production behavior + stale test + * assertion → 3 tests fail. + * + * Two-part fix: + * 1. release-sdk.yml prefix filter now includes test: commits. + * 2. scripts/diff-touches-shipped-paths.cjs treats tests/-rooted paths + * and sdk/src vitest specs (.test.ts / .spec.ts and friends) as + * CI-gating-equivalent so a test: commit that touches only those + * paths passes the shipped-paths gate. + * + * Without both halves the bundling failure mode persists. + */ + +'use strict'; + +// allow-test-rule: source-text-is-the-product +// release-sdk.yml IS the product for hotfix automation; this test reads +// the workflow's prefix-filter regex line directly because the regex IS +// the behavior contract — there is no runtime that consumes it. + +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'); +const { isCiGating } = require('../scripts/diff-touches-shipped-paths.cjs'); + +function workflowText() { + return fs.readFileSync(WORKFLOW_PATH, 'utf8'); +} + +function runClassifier(stdin, cwd) { + return spawnSync(process.execPath, [CLASSIFIER_PATH], { + cwd, + input: stdin, + encoding: 'utf8', + }); +} + +function writeMinimalPkg(tmp, files) { + fs.writeFileSync( + path.join(tmp, 'package.json'), + JSON.stringify({ name: 'test-pkg', version: '0.0.0', files }, null, 2), + ); +} + +describe('bug-3621: release-sdk hotfix accepts `test:` commits', () => { + test('candidate-loop regex includes test: prefix alongside fix:/chore:', () => { + const text = workflowText(); + // The regex must accept all three prefixes — fix, chore, test — with + // optional (scope) and optional ! for breaking. Asserted by extracting + // the literal regex line. + const regexLine = text.split('\n').find((l) => l.includes("grep -qE '^(") && l.includes(')(\\([^)]+\\))?!?: ')); + assert.ok(regexLine, 'release-sdk.yml must contain the candidate-prefix regex line'); + assert.ok( + /\^\(fix\|chore\|test\)/.test(regexLine), + `prefix regex must include fix|chore|test (got: ${regexLine.trim()})`, + ); + }); + + test('regex does NOT silently drop fix: or chore: while adding test:', () => { + const text = workflowText(); + const regexLine = text.split('\n').find((l) => l.includes("grep -qE '^(")); + assert.ok(regexLine.includes('fix|chore|test'), 'must keep fix and chore as before'); + assert.ok(!regexLine.includes('feat|'), 'must NOT silently add feat: — features are not hotfix-eligible'); + assert.ok(!regexLine.includes('docs|'), 'must NOT silently add docs: — docs commits remain POLICY_SKIPPED'); + }); +}); + +describe('bug-3621: shipped-paths classifier treats test paths as CI-gating', () => { + test('isCiGating recognizes tests/** as CI-gating', () => { + assert.equal(isCiGating('tests/bug-foo.test.cjs'), true); + assert.equal(isCiGating('tests/helpers.cjs'), true); + assert.equal(isCiGating('tests/fixtures/adversarial/roadmap/duplicate-keys.md'), true); + }); + + test('isCiGating recognizes sdk/src vitest specs as CI-gating', () => { + assert.equal(isCiGating('sdk/src/query/init.test.ts'), true); + assert.equal(isCiGating('sdk/src/query/init.spec.ts'), true); + assert.equal(isCiGating('sdk/src/golden/golden.integration.test.ts'), true); + }); + + test('isCiGating rejects non-test sdk/src paths (those ship via sdk/dist)', () => { + assert.equal(isCiGating('sdk/src/query/init.ts'), false); + assert.equal(isCiGating('sdk/src/config.ts'), false); + }); + + test('isCiGating rejects paths that merely contain "test" in their name', () => { + // Guard against /test/ false positives. + assert.equal(isCiGating('docs/test-strategy.md'), false); + assert.equal(isCiGating('bin/install-test-helper.js'), false); + // .test.md is not a recognized spec extension here. + assert.equal(isCiGating('docs/install.test.md'), false); + }); + + test('classifier exits 0 for a test-only diff (the v1.42.3 case)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const result = runClassifier('tests/install-minimal-all-runtimes.test.cjs\n', tmp); + assert.equal( + result.status, + 0, + `tests/** path must classify as shipped-equivalent (status=${result.status}, stderr=${result.stderr})`, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('classifier exits 0 for a mixed test+docs diff (the v1.42.3 commit shape)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-mixed-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const stdin = [ + 'README.md', + 'docs/CONFIGURATION.md', + 'docs/USER-GUIDE.md', + 'tests/install-minimal-all-runtimes.test.cjs', + 'tests/installer-migration-install-integration.test.cjs', + ].join('\n') + '\n'; + const result = runClassifier(stdin, tmp); + assert.equal( + result.status, + 0, + `mixed docs+test diff must classify as shipped-equivalent because at least one path is CI-gating (status=${result.status})`, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('classifier still exits 1 for a pure docs-only diff (no test paths)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-docs-only-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const stdin = 'README.md\ndocs/CONFIGURATION.md\n'; + const result = runClassifier(stdin, tmp); + assert.equal( + result.status, + 1, + `pure docs-only diff must remain NOT_SHIPPED — they aren't CI-gating either (status=${result.status})`, + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('classifier still exits 0 for a normal shipped-path diff (pre-fix behavior preserved)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-shipped-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const result = runClassifier('bin/install.js\n', tmp); + assert.equal(result.status, 0, 'shipped-path classification must not regress'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('classifier still exits 1 for CI-only workflow diffs (.github/** is neither shipped nor CI-gating)', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-ci-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const result = runClassifier('.github/workflows/release-sdk.yml\n', tmp); + assert.equal( + result.status, + 1, + 'workflow-only changes must remain NOT_SHIPPED — they\'d otherwise fail the push step (default GITHUB_TOKEN lacks workflow scope, #2980)', + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('push-blocking guard wins: workflow + test bundle classifies as NOT_SHIPPED (preserves #2980)', () => { + // The canonical #2980 case is a `fix(release-sdk):` commit touching + // both the workflow file and its regression test. Under #3621 the test + // path alone would otherwise satisfy the CI-gating check; the + // .github/workflows/* push-blocker must still skip the whole commit. + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-3621-classifier-pushblock-')); + try { + writeMinimalPkg(tmp, ['bin', 'sdk/dist']); + const stdin = [ + '.github/workflows/release-sdk.yml', + 'tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs', + 'CHANGELOG.md', + ].join('\n') + '\n'; + const result = runClassifier(stdin, tmp); + assert.equal( + result.status, + 1, + '#2980 preservation: bundle with .github/workflows/* must skip regardless of test paths in the same commit', + ); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); +});