mirror of
https://github.com/glittercowboy/get-shit-done
synced 2026-05-13 10:36:38 +02:00
* 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.
102 lines
3.9 KiB
JavaScript
102 lines
3.9 KiB
JavaScript
#!/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 <SHA>`) and
|
|
* exits with one of three codes so the workflow can distinguish a
|
|
* legitimate "skip this commit" signal from a classifier failure.
|
|
*
|
|
* "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 (the workflow MUST treat these distinctly — bug #2983):
|
|
* 0 at least one path is shipped → cherry-pick is meaningful
|
|
* 1 no shipped paths → CI / test / docs / planning
|
|
* only; hotfix loop skips
|
|
* 2 classifier error → bad/missing package.json,
|
|
* I/O failure, or any
|
|
* uncaught exception. The
|
|
* workflow MUST fail-fast on
|
|
* this code rather than
|
|
* treating it as a skip.
|
|
*
|
|
* Why distinct codes: Node's default exit code for uncaught throws is 1,
|
|
* which would otherwise be indistinguishable from the legitimate "no
|
|
* shipped paths" result. CodeRabbit on PR #2981 / bug #2983.
|
|
*/
|
|
|
|
'use strict';
|
|
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
|
|
const EXIT_SHIPPED = 0;
|
|
const EXIT_NOT_SHIPPED = 1;
|
|
const EXIT_ERROR = 2;
|
|
|
|
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 fail(message, err) {
|
|
process.stderr.write(`diff-touches-shipped-paths: ${message}\n`);
|
|
if (err && err.stack) process.stderr.write(`${err.stack}\n`);
|
|
process.exit(EXIT_ERROR);
|
|
}
|
|
|
|
function main() {
|
|
// Surface ANY uncaught failure as exit 2 (classifier error) rather
|
|
// than letting Node's default-1 shadow the legitimate
|
|
// "no shipped paths" result. Bug #2983.
|
|
process.on('uncaughtException', (err) => fail('uncaught exception', err));
|
|
process.on('unhandledRejection', (err) => fail('unhandled rejection', err));
|
|
|
|
let shipPrefixes;
|
|
try {
|
|
const pkgPath = path.resolve(process.cwd(), 'package.json');
|
|
shipPrefixes = loadShipPrefixes(pkgPath);
|
|
} catch (err) {
|
|
return fail(`failed to read package.json from ${process.cwd()}`, err);
|
|
}
|
|
|
|
let buf = '';
|
|
process.stdin.setEncoding('utf8');
|
|
process.stdin.on('error', (err) => fail('stdin read error', err));
|
|
process.stdin.on('data', (chunk) => {
|
|
buf += chunk;
|
|
});
|
|
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);
|
|
} catch (err) {
|
|
fail('classification failed', err);
|
|
}
|
|
});
|
|
}
|
|
|
|
if (require.main === module) {
|
|
main();
|
|
}
|
|
|
|
module.exports = { loadShipPrefixes, isShipped, EXIT_SHIPPED, EXIT_NOT_SHIPPED, EXIT_ERROR };
|