diff --git a/.github/workflows/release-sdk.yml b/.github/workflows/release-sdk.yml index 83a31b2b8..8eeea2d20 100644 --- a/.github/workflows/release-sdk.yml +++ b/.github/workflows/release-sdk.yml @@ -165,10 +165,20 @@ jobs: # don't match the fix/chore filter (feat/refactor/docs/etc). # CONFLICT_SKIPPED — fix/chore commits whose cherry-pick failed # and were skipped per the full-automation policy (#2968). + # NON_SHIPPED_SKIPPED — fix/chore 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 + # into a hotfix is meaningless — and picking workflow-file + # changes specifically would also fail the push step because + # the default GITHUB_TOKEN lacks the `workflow` scope. The + # shipped-paths filter is the precise root cause: bug #2980. # Operators reviewing the run summary need these distinct so - # the manual-review queue isn't buried in the policy bucket. + # the manual-review queue (CONFLICT_SKIPPED) isn't buried in + # the noise from the other two buckets. POLICY_SKIPPED="" CONFLICT_SKIPPED="" + NON_SHIPPED_SKIPPED="" while IFS= read -r SHA; do [ -z "$SHA" ] && continue SUBJECT=$(git log -1 --format='%s' "$SHA") @@ -186,6 +196,30 @@ jobs: CONFLICT_SKIPPED="${CONFLICT_SKIPPED}- \`${SHA}\` ${SUBJECT} ($REASON)"$'\n' continue fi + # Pre-pick guard: a hotfix release can only be affected + # by commits whose diff intersects the npm tarball's + # shipped paths (package.json `files` whitelist plus + # package.json itself, which `npm pack` always + # includes). Commits that touch only CI workflows, + # tests, docs, or planning artifacts cannot change what + # ships, so picking them into a hotfix is meaningless. + # As a side benefit, this excludes + # `.github/workflows/*` changes whose push would + # otherwise be rejected by GitHub because the default + # GITHUB_TOKEN lacks the `workflow` scope. The filter + # is implemented in + # scripts/diff-touches-shipped-paths.cjs rather than + # inline so the rules (read package.json `files`, + # 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 + fi echo "→ cherry-picking $SHA $SUBJECT" # Pin merge.conflictStyle=merge on the cherry-pick so the # awk classifier below sees deterministic marker shapes — @@ -277,6 +311,13 @@ jobs: else echo "_No fix/chore 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 "" + echo "$NON_SHIPPED_SKIPPED" + fi if [ -n "$CONFLICT_SKIPPED" ]; then echo "### Skipped — cherry-pick conflict (manual review)" echo "" diff --git a/CHANGELOG.md b/CHANGELOG.md index 5522cacf9..fc47e3116 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 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) - **`sketch --wrap-up` now dispatches correctly** — `/gsd-sketch --wrap-up` was silently no-oping because the flag dispatch wiring was omitted when the micro-skill entry point was absorbed in #2790. (#2949) diff --git a/scripts/diff-touches-shipped-paths.cjs b/scripts/diff-touches-shipped-paths.cjs new file mode 100644 index 000000000..1002e16e8 --- /dev/null +++ b/scripts/diff-touches-shipped-paths.cjs @@ -0,0 +1,65 @@ +#!/usr/bin/env node +/** + * Used by the release-sdk hotfix cherry-pick loop to decide whether a + * candidate commit can possibly change what ships in the npm package. + * + * 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. + * + * "Shipped" = the union of: + * - 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). + * + * `package-lock.json` is intentionally NOT considered shipped — `npm pack` + * 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 + */ + +'use strict'; + +const fs = require('node:fs'); +const path = require('node:path'); + +function loadShipPrefixes(pkgPath) { + const pkg = JSON.parse(fs.readFileSync(pkgPath, 'utf8')); + const files = Array.isArray(pkg.files) ? pkg.files : []; + return ['package.json', ...files]; +} + +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 + // tool's output shouldn't get a false negative). + const p = diffPath.replace(/\\/g, '/'); + return shipPrefixes.some((s) => p === s || p.startsWith(s + '/')); +} + +function main() { + const pkgPath = path.resolve(process.cwd(), 'package.json'); + const shipPrefixes = loadShipPrefixes(pkgPath); + + let buf = ''; + process.stdin.setEncoding('utf8'); + 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); + }); +} + +if (require.main === module) { + main(); +} + +module.exports = { loadShipPrefixes, isShipped }; 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 8eda40d1c..66ea3b782 100644 --- a/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs +++ b/tests/bug-2964-release-sdk-empty-cherry-pick.test.cjs @@ -71,15 +71,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 to ~4 KB after the anchor — generous enough that future - // pre-skip guards / classification scaffolding (e.g. the merge-commit - // pre-skip added on PR #2970) don't push the call out of range, but - // still tight enough to avoid matching unrelated cherry-pick refs - // elsewhere in the workflow file. + // 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. // 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 + 4000); + const window = yaml.slice(loopAnchor, loopAnchor + 6000); 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 new file mode 100644 index 000000000..c61872422 --- /dev/null +++ b/tests/bug-2980-hotfix-only-picks-shipping-changes.test.cjs @@ -0,0 +1,298 @@ +/** + * Regression test for bug #2980 + * + * The release-sdk hotfix cherry-pick loop's `fix:`/`chore:` filter is + * too broad: it picks anything with that conventional-commit type + * regardless of whether the diff can affect the published npm package. + * That caused two compounding problems: + * + * 1. CI-only fixes (release-sdk.yml, hotfix tooling) were cherry-picked + * into hotfix branches even though they cannot change what ships. + * 2. The subset of those CI-only fixes touching `.github/workflows/*` + * caused the prepare job's `git push` to be rejected by GitHub — + * the default GITHUB_TOKEN lacks the `workflow` scope: + * + * ! [remote rejected] hotfix/X.YY.Z -> hotfix/X.YY.Z + * (refusing to allow a GitHub App to create or update workflow + * ... without `workflows` permission) + * + * v1.39.1 hit this on PR #2977 (run 25232010071): #2977 cherry- + * picked cleanly because earlier workflow-file fixes had been + * skipped on conflict, then the push exploded. + * + * Fix (root cause): pre-pick guard that checks whether the candidate + * commit's diff intersects the npm tarball's shipped paths (entries in + * `package.json` `files` plus `package.json` itself). Non-shipping + * commits are skipped with an informational summary entry; the + * workflow-file rejection is now a non-issue because workflow files + * are not in `files`. + * + * The shipped-paths classifier lives in + * `scripts/diff-touches-shipped-paths.cjs` rather than inline in the + * workflow YAML so its rules are unit-testable. + * + * This test covers two layers: + * - Static workflow assertions (the loop calls the script before + * attempting the pick, the result drives a NON_SHIPPED_SKIPPED + * bucket, and the run summary surfaces it). + * - Behavioral assertions on the classifier script itself (matches + * `npm pack` semantics for `files` entries). + */ + +'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`); +} + +/** + * Slice the lines from the merge-commit pre-skip guard up to (but not + * including) the cherry-pick attempt. Any new pre-pick guard MUST live + * in this region to fire before the pick. + */ +function extractPrePickRegion(script) { + const lines = script.split('\n'); + const startIdx = lines.findIndex(l => /merge commit — manual -m parent selection required/.test(l)); + if (startIdx === -1) throw new Error('merge-commit pre-skip guard not found — sentinel for pre-pick region'); + const endIdx = lines.findIndex((l, i) => i > startIdx && /git[^\n]*cherry-pick[^\n]*"\$SHA"/.test(l)); + if (endIdx === -1) throw new Error('cherry-pick attempt not found after merge-commit guard'); + return lines.slice(startIdx, endIdx).join('\n'); +} + +describe('bug-2980: release-sdk hotfix only picks commits that touch shipped paths', () => { + test('pre-pick guard runs the shipped-paths classifier before attempting the pick', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + const prePick = extractPrePickRegion(script); + + // Must call the classifier script. Inline grep on `.github/workflows/` + // would only catch the workflow-file subset of the bug — the broader + // root cause is "any non-shipping commit in a hotfix is meaningless" + // and the classifier encodes the precise `files`-whitelist rule. + assert.match( + prePick, + /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)' + ); + 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)' + ); + // 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. + 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)' + ); + }); + + test('non-shipped skips land in NON_SHIPPED_SKIPPED, distinct from CONFLICT_SKIPPED and POLICY_SKIPPED', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + const prePick = extractPrePickRegion(script); + + assert.match( + prePick, + /^\s*NON_SHIPPED_SKIPPED="\$\{NON_SHIPPED_SKIPPED\}/m, + 'non-shipped skip must append to NON_SHIPPED_SKIPPED — distinct from CONFLICT_SKIPPED (manual-review queue) and POLICY_SKIPPED (feat/refactor exclusions) (#2980)' + ); + // The bucket must be initialized at the top of the loop alongside + // the other two — so a future `set -u` doesn't silently break it. + assert.match( + script, + /^\s*NON_SHIPPED_SKIPPED=""\s*$/m, + 'NON_SHIPPED_SKIPPED must be initialized to empty alongside POLICY_SKIPPED and CONFLICT_SKIPPED (#2980)' + ); + }); + + test('non-shipped skip emits no ::warning:: — the change cannot affect the package', () => { + // A non-shipped commit is by definition incapable of changing what + // ships, so the skip needs no operator alert. The summary bucket is + // informational; a yellow warning would imply remediation is + // possible, which would mislead operators. + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + const prePick = extractPrePickRegion(script); + + assert.doesNotMatch( + prePick, + /::warning::/, + 'non-shipped skip must NOT emit a ::warning:: — the commit cannot change what ships, so a warning would falsely imply remediation is needed (#2980)' + ); + }); + + test('run summary surfaces NON_SHIPPED_SKIPPED in its own section, framed as informational', () => { + const yaml = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const script = extractStepRun(yaml, 'Prepare hotfix branch'); + + assert.match( + script, + /if \[ -n "\$NON_SHIPPED_SKIPPED" \]/, + 'run summary must conditionally render the NON_SHIPPED_SKIPPED bucket so empty hotfixes don\'t print an empty section (#2980)' + ); + // The header must NOT use "manual review" framing — that's the + // CONFLICT_SKIPPED queue. Non-shipped skips need no manual action. + assert.doesNotMatch( + script, + /Skipped — touches no shipped paths[^\n]*manual review/, + 'NON_SHIPPED_SKIPPED summary header must NOT imply manual review — non-shipping commits need no remediation (#2980)' + ); + assert.match( + script, + /Skipped — touches no shipped paths[^\n]*informational/, + 'NON_SHIPPED_SKIPPED summary header must signal "informational" so operators don\'t mistake it for the manual-review queue (#2980)' + ); + }); +}); + +describe('bug-2980: scripts/diff-touches-shipped-paths.cjs classifier semantics', () => { + function runClassifier(stdin, cwd) { + return spawnSync('node', [CLASSIFIER_PATH], { + cwd, + input: stdin, + encoding: 'utf8', + }); + } + + function makeFixtureRepo(filesArray) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bug-2980-')); + fs.writeFileSync( + path.join(tmp, 'package.json'), + JSON.stringify({ name: 'fixture', version: '0.0.0', files: filesArray }, null, 2) + ); + return tmp; + } + + test('directory entry in `files` matches paths under that directory but not sibling prefixes', () => { + const tmp = makeFixtureRepo(['bin', 'sdk/dist']); + try { + // bin/foo.js is shipped (under bin/). + assert.equal(runClassifier('bin/foo.js\n', tmp).status, 0, 'bin/foo.js must be shipped'); + // bin alone (the directory entry itself) is shipped. + assert.equal(runClassifier('bin\n', tmp).status, 0, 'bin (exact match) must be shipped'); + // binaries/foo.js must NOT match bin (prefix-without-slash bug). + assert.equal(runClassifier('binaries/foo.js\n', tmp).status, 1, 'binaries/foo.js must NOT match bin/ — prefix without slash boundary is a classic bug'); + // sdk/dist/cli.js is shipped. + assert.equal(runClassifier('sdk/dist/cli.js\n', tmp).status, 0, 'sdk/dist/cli.js must be shipped'); + // sdk/src/cli.ts is NOT shipped (only sdk/dist is in `files`). + assert.equal(runClassifier('sdk/src/cli.ts\n', tmp).status, 1, 'sdk/src/cli.ts must NOT be shipped when only sdk/dist is whitelisted'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('package.json is always shipped even when not in `files`', () => { + // `npm pack` always includes package.json regardless of `files`. The + // classifier must mirror that, so a version-bump-only commit isn't + // wrongly skipped. + const tmp = makeFixtureRepo([]); + try { + assert.equal(runClassifier('package.json\n', tmp).status, 0, 'package.json must be classified as shipped — `npm pack` always includes it'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('package-lock.json is NOT shipped unless explicitly in `files`', () => { + // `npm pack` does NOT include package-lock.json by default. A + // lockfile-only commit can't change the published package's runtime + // behavior (consumers resolve their own lockfile from `dependencies`). + const tmp = makeFixtureRepo(['bin']); + try { + assert.equal(runClassifier('package-lock.json\n', tmp).status, 1, 'package-lock.json must NOT be classified as shipped when absent from `files` — `npm pack` excludes it by default'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + 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. + const tmp = makeFixtureRepo(['bin']); + try { + const stdin = '.github/workflows/release-sdk.yml\nbin/foo.js\ntests/bar.test.cjs\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 }); + } + }); + + test('purely CI/test/docs commit is NOT shipped (the actual #2980 case)', () => { + // The classic case: a fix(release-sdk): commit that touches only + // .github/workflows/release-sdk.yml and a regression test under + // tests/. Pre-#2980 the loop picked it; the cherry-pick succeeded; + // the push then failed because of the workflow-file scope rule. + // Post-#2980 the loop skips it pre-pick — the push problem and the + // "meaningless pick" problem dissolve together. + const tmp = makeFixtureRepo(['bin', 'commands', 'sdk/dist']); + try { + const stdin = '.github/workflows/release-sdk.yml\ntests/bug-2980-shipped-paths.test.cjs\nCHANGELOG.md\n'; + assert.equal(runClassifier(stdin, tmp).status, 1, 'CI-only commit (workflow + test + changelog) must classify as NOT shipped — the canonical #2980 case'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); + + test('empty stdin classifies as not-shipped (defensive — empty diff means no candidate paths)', () => { + const tmp = makeFixtureRepo(['bin']); + try { + assert.equal(runClassifier('', tmp).status, 1, 'empty stdin must classify as not-shipped — no paths can\'t intersect any whitelist'); + assert.equal(runClassifier('\n\n\n', tmp).status, 1, 'whitespace-only stdin must classify as not-shipped'); + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); +});