diff --git a/.github/workflows/pr-automation.yml b/.github/workflows/pr-automation.yml index 274cab3f00d..e0b36ccdfa0 100644 --- a/.github/workflows/pr-automation.yml +++ b/.github/workflows/pr-automation.yml @@ -739,8 +739,9 @@ jobs: This is said HERE rather than in that refusal on purpose. The step below prints the two-class remedy (COLLISION vs DELIBERATE CORRECTION), but on this class it never speaks: a PR that only corrects somebody else's note - adds none of its own, so THIS step fails first and the steps after it - are skipped. + adds none of its own, so THIS step fails first and that step is skipped. + The ADR-0087 disposition and major-bump steps after it still run and + judge the note you changed -- read their verdicts on this run too. A PR that corrects a pending note AND releases something of its own does both: route 1 for its own release, under a name that cannot collide @@ -929,10 +930,49 @@ jobs: # `check-*.mjs` gates: its fixtures are the only place the red directions are # pinned, and each of them was verified to flip green when the corresponding # check is ablated. Real temp git repositories, well under a second. + # + # ── Past a designed red (#20784) ───────────────────────────────────────── + # + # This step and the two after it (the allow-major read and the major guard) + # name `!cancelled()`, so they run whatever the steps above concluded. Two + # steps above are RED BY DESIGN on the DELIBERATE CORRECTION class, and the + # red is left standing on purpose: `Require a changeset` on a PR that only + # corrects somebody else's pending note (route 0), and the empty-changeset + # step on one that corrects a note AND adds its own (the foreign-changeset + # refusal). Under GitHub's implicit `success()` either red skipped all three + # verdicts below, on exactly the PRs most likely to carry a breaking + # changeset, so neither the ADR-0087 disposition nor the level axis was ever + # read on them in CI. Both gates judge `M` rows as well as `A`, so a + # correction-only PR has something for them to judge. The job stays red on + # that class; it now reports every verdict on the same run. + # + # ⛔ Not `always()`: a cancelled run must still stop. + # + # `steps.diffbase.outputs.merge_base != ''` is what `!cancelled()` costs, + # and it is not optional. Under the implicit `success()` an unusable base + # skipped these steps too, so a non-empty `$MERGE_BASE` came for free. Run + # past a red, they would also run past `Require a usable diff base`, where + # the base is the empty string, and all three changeset scripts read + # `--base ""` as NO base and answer against `origin/main` at exit 0 + # (measured). That is the base-free reading #6129 and #7005 removed: a + # verdict about a base this job refused to trust. So each of the three names + # the input it consumes. Nothing else needs a guard: the two scripts import + # only node builtins and repo-local modules, so a failed install cannot + # change their verdict, and an unwritten label output is not 'true', which + # is the enforcing direction every read in this job already takes (#4690). + # + # The set is CLOSED. Each step above reads an output that an earlier step + # may not have written (the count, the settled label), so each keeps the + # implicit `success()`: `Require a changeset` run past a failed count would + # test an empty `$ADDED` and announce a green (see the note on `Require a + # usable diff base`). check-empty-changeset.mjs's consumer battery pins + # this shape: the three steps, their two conjuncts, and no `always()`. - name: Require an ADR-0087 disposition on a declared-breaking changeset if: >- - steps.labels.outputs.skip != 'true' + !cancelled() + && steps.labels.outputs.skip != 'true' && steps.labels_settled.outputs.skip != 'true' + && steps.diffbase.outputs.merge_base != '' env: MERGE_BASE: ${{ steps.diffbase.outputs.merge_base }} run: | @@ -1005,10 +1045,16 @@ jobs: id: allow_major # Both skip-changeset reads, for the same reason the guard below names # them: a PR the changeset gate exempts must not pay an API call for a - # guard that is not going to run. + # guard that is not going to run. The same argument gives it the guard's + # other two conjuncts: `!cancelled()`, because this read is the guard's + # input and the guard runs past a designed red (#20784, the note on the + # ADR-0087 step), and the `merge_base` read, because without a base the + # guard does not run and this call would buy nothing. if: >- - steps.labels.outputs.skip != 'true' + !cancelled() + && steps.labels.outputs.skip != 'true' && steps.labels_settled.outputs.skip != 'true' + && steps.diffbase.outputs.merge_base != '' env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} @@ -1037,17 +1083,24 @@ jobs: # version. During the launch window we ship breaking changes as `minor`. # Add the `allow-major` PR label when a whole-stack major is intended. # - # The first two clauses keep this step exempt exactly when the changeset - # check above is: before #5580 the `skip-changeset` label skipped the whole - # job, this step included, and a live-read label must not quietly re-arm - # it. Both reads are named for the same reason -- after #6378 the exemption - # can be established by either one, and a step that honoured only the fast - # path would re-arm itself on precisely the PRs the settling read rescued. + # `!cancelled()` and the `merge_base` clause are the pair the note on the + # ADR-0087 step argues (#20784): this verdict is read even when a step + # above is red by design, and never against a base this job could not + # resolve. ⛔ Not `always()`. # - # The third clause is the LIVE `allow-major` read directly above (#5620), - # never the event payload. Its comment block carries the argument: what the - # payload read cost, where this diverges from #5580's `skip-changeset` read - # and why, the residual it leaves, and what re-arms the whole thing. + # The two skip-changeset clauses keep this step exempt exactly when the + # changeset check above is: before #5580 the `skip-changeset` label skipped + # the whole job, this step included, and a live-read label must not + # quietly re-arm it. Both reads are named for the same reason -- after + # #6378 the exemption can be established by either one, and a step that + # honoured only the fast path would re-arm itself on precisely the PRs the + # settling read rescued. + # + # The `allow_major` clause is the LIVE `allow-major` read directly above + # (#5620), never the event payload. Its comment block carries the + # argument: what the payload read cost, where this diverges from #5580's + # `skip-changeset` read and why, the residual it leaves, and what re-arms + # the whole thing. # # `--base` takes the same merge base the two gates above take, for the # same #6129 reason one defect along (#7005). This script used to read @@ -1067,8 +1120,10 @@ jobs: # without the fork -- #6129's argument, which never depended on how big # the stock was. if: >- - steps.labels.outputs.skip != 'true' + !cancelled() + && steps.labels.outputs.skip != 'true' && steps.labels_settled.outputs.skip != 'true' + && steps.diffbase.outputs.merge_base != '' && steps.allow_major.outputs.allow != 'true' env: MERGE_BASE: ${{ steps.diffbase.outputs.merge_base }} diff --git a/scripts/check-empty-changeset.mjs b/scripts/check-empty-changeset.mjs index b95f9aefd8e..3babd36f1ce 100644 --- a/scripts/check-empty-changeset.mjs +++ b/scripts/check-empty-changeset.mjs @@ -690,7 +690,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ "A' (#17712): a changeset the PR did not add is neither modified nor deleted": 29, 'D (#18160): the refusal names BOTH classes, body and annotation pinned equal': 12, '#4690, one step later: no merge base at all is a failure': 1, - 'The consumer: this gate\'s own CI step (#6129)': 23, + 'The consumer: this gate\'s own CI step (#6129)': 34, 'The second consumer: where THIS SELF-TEST runs (#6509)': 12, 'Parser unit rows': 8, 'THE FIX (#7004): comments and quoted bump values': 12, @@ -1745,6 +1745,77 @@ function selfTest() { `consumer: the live allow-major read must write \`allow=true\` exactly once and only after the label was really observed (found ${allowTrueAt.length} at ${JSON.stringify(allowTrueAt)}, grep at ${grepAt}) -- an exemption handed out because the label list could not be read is the #4690 anti-pattern, and here it would wave a whole-stack major through`, ); + // ── Past a designed red (#20784) ───────────────────────────────────────── + // + // Two steps of this job are red BY DESIGN on the DELIBERATE CORRECTION + // class, and the red is left standing: route 0 of `Require a changeset`, + // and this gate's own foreign-changeset refusal. The three steps after + // this gate -- the ADR-0087 disposition, the live allow-major read that + // feeds the major guard, and the major guard itself -- are independent + // verdicts on the same diff. Under GitHub's implicit `success()` a designed + // red above them skipped all three, so the PRs that correct a pending note + // (the ones most likely to carry a breaking changeset) went through CI with + // neither verdict read, and every assertion in this battery stayed green. + // + // What is pinned, read as one sentence: each of the three LEADS its `if:` + // with `!cancelled() &&` and carries no `||` (so no disjunct can route + // around it); each also requires `steps.diffbase.outputs.merge_base != ''`, + // because a step run past the unusable-base red would hand its script + // `--base ""`, which this script and its two siblings read as NO base and + // answer against `origin/main` at exit 0; the steps of this job that name a + // status function are exactly these three, because every step above them + // reads an output an earlier step may not have written; and nothing in the + // workflow uses `always()`, because a cancelled run must still stop. + const ifOf = (step) => { + const lines = step.split('\n'); + const at = lines.findIndex((l) => /^ {8}if:/.test(l)); + if (at === -1) return null; + const head = lines[at].replace(/^ {8}if:\s*/, '').trim(); + if (!/^[>|][+-]?$/.test(head)) return head; + const body = []; + for (const l of lines.slice(at + 1)) { + if (l.trim() !== '' && !/^ {9,}/.test(l)) break; + body.push(l.trim()); + } + return body.filter(Boolean).join(' '); + }; + const pastRed = [ + ['the ADR-0087 disposition', (c) => /node scripts\/check-adr-0087-registration\.mjs --base/.test(c)], + ['the live allow-major read', (c) => /grep -qxF 'allow-major'/.test(c)], + ['the major guard', (c) => /node scripts\/check-changeset-no-major\.mjs --base/.test(c)], + ].map(([label, isIt]) => { + const at = namedSteps.findIndex(isIt); + return { label, at, cond: at === -1 ? null : ifOf(namedSteps[at]) }; + }); + for (const { label, cond } of pastRed) { + assert( + cond !== null, + `consumer: ${label} step could not be sliced out of the Check Changeset job with an \`if:\` -- the two assertions after this one would judge nothing`, + ); + assert( + /^!cancelled\(\)\s*&&/.test(cond ?? '') && !/\|\|/.test(cond ?? ''), + `consumer: ${label} step must lead its \`if:\` with \`!cancelled() &&\` and carry no \`||\` -- under the implicit \`success()\` a designed red above it (route 0, or the foreign-changeset refusal) skips it, and the PRs that correct a pending note get no verdict from it in CI. Got ${JSON.stringify(cond)}`, + ); + assert( + /steps\.diffbase\.outputs\.merge_base != ''/.test(cond ?? ''), + `consumer: ${label} step must require \`steps.diffbase.outputs.merge_base != ''\` -- run past a red it also runs past the unusable-base red, and a changeset script handed \`--base ""\` reads NO base and answers against origin/main at exit 0. Got ${JSON.stringify(cond)}`, + ); + } + const NAMES_STATUS = /\b(?:always|cancelled|success|failure)\s*\(\s*\)/; + const namingStatus = namedSteps + .map((c, i) => ({ i, cond: ifOf(c) })) + .filter(({ cond }) => NAMES_STATUS.test(cond ?? '')) + .map(({ i }) => i); + const expectedStatus = pastRed.map(({ at }) => at).sort((a, b) => a - b); + assert( + expectedStatus.every((i) => i >= 0) && JSON.stringify(namingStatus) === JSON.stringify(expectedStatus), + `consumer: in the Check Changeset job exactly the three steps after the empty-changeset gate may name a status function (steps ${JSON.stringify(expectedStatus)}), found ${JSON.stringify(namingStatus)} -- every step above them reads an output an earlier step may not have written, and run past a red one of them announces a verdict on an empty input`, + ); + assert( + !/\balways\s*\(\s*\)/.test(yaml.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n')), + 'consumer: nothing in pr-automation.yml may use `always()` -- `!cancelled()` runs a step past a red, `always()` also runs it on a cancelled run, which must still stop', + ); + // The hard constraint of #6378, stated as structure: none of this may have // made the gate softer. A PR with no changeset and no label still has to // hit a real non-zero exit, and no step of this job may be excused from