diff --git a/.github/workflows/pr-automation.yml b/.github/workflows/pr-automation.yml index 71190ac2fc..e5dcc511ee 100644 --- a/.github/workflows/pr-automation.yml +++ b/.github/workflows/pr-automation.yml @@ -274,6 +274,23 @@ jobs: # someone gives the checkout a `ref:`, where parent^1 becomes the PR's # previous commit. `merge-base` is right under BOTH checkouts, which is # why it is the one written here. + # + # #6434: this step REPORTS an unusable base, it does not adjudicate one. + # Both of its former `exit 1`s are now `base_error` outputs, and the verdict + # moved to `Require a usable diff base` further down -- the one place that + # has seen BOTH label reads. The reason is the same #5580/#6378 race one + # defect along: this step runs before the settling read, so a + # `skip-changeset` label that lands in its window (the COMMON case, ~+10..45s + # from PR creation) was still invisible here. Failing on the spot therefore + # reddened a PR the job was about to exempt -- two conditions at once, the + # late label plus a genuine git-base failure, which is why no instance was + # ever observed and why the gap was recorded rather than guessed at. + # + # What is deliberately NOT changed: an unusable base is still a FAILURE for + # every PR the changeset gate applies to. #4690 governs the verdict, not its + # address. The warning below is emitted where the fact is discovered so the + # infrastructure flake stays visible in the log even on the exempt path, + # where nothing else would ever mention it. - name: Resolve the diff base (merge base with the base branch) id: diffbase if: steps.labels.outputs.skip != 'true' @@ -282,8 +299,9 @@ jobs: PINNED_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: | if [ -z "$BASE_REF" ]; then - echo "::error::This event carries no base branch, so the changeset diff base cannot be computed. A gate that cannot read its input has verified nothing, so this is a failure rather than a pass (#4690)." - exit 1 + echo "::warning::This event carries no base branch, so the changeset diff base cannot be computed. Adjudicated below, once the skip-changeset window has settled." + echo 'base_error=This event carries no base branch, so the changeset diff base cannot be computed.' >> "$GITHUB_OUTPUT" + exit 0 fi if ! git rev-parse --verify --quiet "refs/remotes/origin/$BASE_REF^{commit}" >/dev/null; then git fetch --no-tags --quiet origin "+refs/heads/$BASE_REF:refs/remotes/origin/$BASE_REF" \ @@ -292,11 +310,17 @@ jobs: # `if !` rather than a bare assignment on purpose: these steps run under # `bash -e` (no `shell:` key anywhere in this file), where a failing # command substitution kills the step with no message at all. The gate - # is allowed to fail here -- it is NOT allowed to fail unexplained. + # is allowed to fail over this -- it is NOT allowed to fail unexplained. if ! MERGE_BASE=$(git merge-base "refs/remotes/origin/$BASE_REF" HEAD); then - echo "::error::Could not compute merge-base(origin/$BASE_REF, HEAD), so the changeset diff has no trustworthy starting point. Failing rather than falling back to the frozen base.sha, which is the #6129 defect itself." - exit 1 + echo "::warning::Could not compute merge-base(origin/$BASE_REF, HEAD), so the changeset diff has no trustworthy starting point. Adjudicated below, once the skip-changeset window has settled." + echo "base_error=Could not compute merge-base(origin/$BASE_REF, HEAD), so the changeset diff has no trustworthy starting point." >> "$GITHUB_OUTPUT" + exit 0 fi + # Written explicitly rather than left unset. An unwritten output already + # reads as the empty string, but every downstream `if:` here spells the + # success case as `base_error == ''`, and a contract that load-bearing + # should be visible in the shell that establishes it. + echo 'base_error=' >> "$GITHUB_OUTPUT" echo "merge_base=$MERGE_BASE" >> "$GITHUB_OUTPUT" # The drift is printed, not just corrected. #6129 was invisible for as # long as it was because nothing in the log ever said which commit the @@ -306,18 +330,37 @@ jobs: echo "Diff base: $MERGE_BASE (merge-base of origin/$BASE_REF and HEAD)" echo "Frozen payload base.sha: $PINNED_BASE_SHA -- $BASE_REF has moved $DRIFT commit(s) since it was frozen, and that drift is exactly what this gate used to count as this PR's own." + # The three toolchain steps and the counting step below all carry the + # `base_error == ''` conjunct as well (#6434). For the counting step it is + # CORRECTNESS: handed an empty `$MERGE_BASE`, `git diff --diff-filter=A "" + # HEAD` fails, but it fails inside a pipeline whose last command is `tr`, so + # the step would report `added=0` -- a fabricated count that reads exactly + # like "this PR forgot its changeset" and would send the verdict step to the + # wrong error message entirely. For the three toolchain steps it is COST and + # legibility: an unusable base can no longer be adjudicated until the label + # window settles, and there is no reason to buy a Node toolchain and a + # `--frozen-lockfile` install first for a run that will either be exempted or + # failed without ever compiling anything. It also keeps the failure signature + # single: on this path the only red is the adjudication step, never an + # install that happened to flake on the same shaky network. - name: Setup Node.js - if: steps.labels.outputs.skip != 'true' + if: >- + steps.labels.outputs.skip != 'true' + && steps.diffbase.outputs.base_error == '' uses: actions/setup-node@v7 with: node-version: '22' - name: Enable Corepack - if: steps.labels.outputs.skip != 'true' + if: >- + steps.labels.outputs.skip != 'true' + && steps.diffbase.outputs.base_error == '' run: corepack enable - name: Install dependencies - if: steps.labels.outputs.skip != 'true' + if: >- + steps.labels.outputs.skip != 'true' + && steps.diffbase.outputs.base_error == '' run: pnpm install --frozen-lockfile # COUNTING ONLY -- the verdict is two steps down (#6378). The split is not @@ -327,7 +370,9 @@ jobs: # that fact before any waiting is considered. - name: Count the changesets this PR adds id: changeset_count - if: steps.labels.outputs.skip != 'true' + if: >- + steps.labels.outputs.skip != 'true' + && steps.diffbase.outputs.base_error == '' env: MERGE_BASE: ${{ steps.diffbase.outputs.merge_base }} run: | @@ -405,11 +450,21 @@ jobs: # already behind us, and the loop does exactly one read and no sleeping. # Only a genuinely fresh PR can wait at all. 120s is ~3x the worst label # latency measured on this repo (~40s, #6358). + # + # #6434 adds the second disjunct, and it is the SAME invariant rather than a + # widening of it: "charge the wait only to a PR that is headed for red". A PR + # whose diff base could not be resolved is headed for red just as surely as + # one that added no changeset -- it simply gets there by the other route, and + # the counting step is skipped on that path, so `added` is `''` and the first + # disjunct can never speak for it. No PR waits that would not have waited + # under #6378: a resolvable base plus a changeset still skips this step + # entirely, at no wall time and no API call. - name: Settle the skip-changeset window (only when this PR would otherwise fail) id: labels_settled if: >- steps.labels.outputs.skip != 'true' - && steps.changeset_count.outputs.added == '0' + && (steps.changeset_count.outputs.added == '0' + || steps.diffbase.outputs.base_error != '') env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} @@ -464,6 +519,37 @@ jobs: echo "Label window closed after $ATTEMPT read(s). Labels on PR #$PR_NUMBER right now: ${LABELS:-(none)}" echo 'skip=false' >> "$GITHUB_OUTPUT" + # #6434: the verdict on an unusable diff base. It is a SEPARATE step rather + # than a branch inside the step below, for the reason this job already gives + # for splitting `Reject an empty-frontmatter changeset` off: the two failures + # are different facts and deserve different messages. "The git base is + # unavailable" is not "you forgot a changeset", and an author who reads the + # second when the first is true will go looking in the wrong place. + # + # It sits HERE, immediately after the settling read, because both of its + # neighbours constrain it. It cannot move up: above the settling read there + # is no settled label state, which is the whole defect being fixed. It cannot + # move down: the step below evaluates `[ "$ADDED" -eq 0 ]` on an `ADDED` that + # is the empty string whenever the counting step was skipped, and `test` + # answering "not a number" is a NON-zero status, so the `if` would take its + # else branch and announce "This PR adds changeset(s)" -- a green verdict on + # a gate that never ran. The ordering is load-bearing in both directions. + # + # This is NOT the route-2 move #6378's cost argument rules out. The settling + # read stays exactly where it is, after the count; nothing about which PRs + # pay for the window changed. What moved is one verdict, downward, past a + # read that had already been paid for. + - name: Require a usable diff base (adjudicated after the label window) + if: >- + steps.labels.outputs.skip != 'true' + && steps.labels_settled.outputs.skip != 'true' + && steps.diffbase.outputs.base_error != '' + env: + BASE_ERROR: ${{ steps.diffbase.outputs.base_error }} + run: | + echo "::error::$BASE_ERROR Failing rather than falling back to the frozen base.sha, which is the #6129 defect itself: a gate that cannot read its input has verified nothing, so this is a failure rather than a pass (#4690). Neither live label read found 'skip-changeset' on this PR, so the changeset gate does apply to it and cannot be evaluated. If this PR releases nothing, apply the label and re-run; if the base branch is genuinely fetchable, this is infrastructure and a re-run will clear it." + exit 1 + # The VERDICT. Everything it needs was decided above; this step only # announces it, which is what makes the failure message a single block of # prose rather than something interleaved with counting and polling. diff --git a/scripts/check-empty-changeset.mjs b/scripts/check-empty-changeset.mjs index e307ae88d6..9be9d2c954 100644 --- a/scripts/check-empty-changeset.mjs +++ b/scripts/check-empty-changeset.mjs @@ -733,6 +733,20 @@ function selfTest() { settle, 'consumer: the settling label read must be conditioned on `steps.changeset_count.outputs.added == \'0\'` -- the wait #6378 introduces is charged ONLY to a PR headed for red, and un-conditioning it taxes every run instead', ); + // The condition above is a substring test, so it would go on passing if a + // later edit bolted an unrelated `|| ` onto it and quietly taxed + // every run again. #6434 legitimately adds ONE disjunct -- the unusable diff + // base, which is the other way a PR arrives at red and which the count + // cannot speak for, because the counting step is skipped on that path. So + // the whole condition is pinned, not just its first term: exactly these two + // ways in, and no third without an argument. + const settleIf = yaml.match( + /steps\.labels\.outputs\.skip != 'true'\s*\n\s*&& \(steps\.changeset_count\.outputs\.added == '0'\s*\n\s*\|\| steps\.diffbase\.outputs\.base_error != ''\)/, + ); + assert( + settleIf !== null, + "consumer: the settling read's condition must be exactly `no fast-path label AND (added == '0' OR base_error != '')` -- both disjuncts are ways of being headed for RED, which is the only thing that may buy the #6378 wait", + ); // Both reads, one matcher. A divergence (say a substring `grep -q` on one // path) would be a gate that exempts on one read and enforces on the @@ -744,20 +758,56 @@ function selfTest() { `consumer: exactly two live \`grep -qxF 'skip-changeset'\` reads are expected (the fast path and the settling read); found ${matchers.length}`, ); - // Every step that can FAIL a PR over the changeset rule must honour both - // reads. Scoped to those steps by what they run, not by name: a step that - // shells out to a `check-*.mjs` gate, or that emits the "no changeset" - // error. `Resolve the diff base` is deliberately outside this set -- it - // exits 1 over an unusable git base, which is not a changeset verdict and - // was never label-exempt (recorded, not implied). + // Every step of this job that can FAIL a PR must honour both reads. Scoped + // by what a step RUNS, never by its name: it shells out to a `check-*.mjs` + // gate, or it contains a literal `exit 1` outside a comment. + // + // #6434 widened this boundary, and the widening is the point rather than an + // accident of it. The predicate used to be "runs a `check-*.mjs` OR emits + // the no-changeset error", which described the four steps that existed and + // nothing else. `Resolve the diff base` sat outside it carrying two `exit + // 1`s of its own -- deliberately, on the argument that "the git base is + // unavailable" is not a changeset verdict and so was never label-exempt. + // That argument is sound about the VERDICT and wrong about its ADDRESS: the + // step ran before the settling read, so on a PR whose `skip-changeset` label + // landed in the ordinary +10..45s window it could red a run the job was + // about to exempt. #6434 moved that verdict to `Require a usable diff base`, + // which honours both reads, and the boundary here moved with it. + // + // Naming `exit 1` instead of one specific error string is what makes the + // rule outlive the steps it was written for: the gap #6434 closed existed + // precisely because the old predicate could not see a failing step it had + // not been told about, and the next one added here would have been invisible + // the same way. Residual, stated rather than implied: a step that fails by + // running a command that returns non-zero, with no literal `exit 1` and no + // `check-*.mjs`, is still outside this set. That is a smaller hole than the + // one it replaces, not no hole. const jobText = yaml.slice(yaml.indexOf('\n changeset-check:')); const chunks = jobText .split(/\n(?= - name: )/) .map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n')) - .filter((c) => /node scripts\/check-\S+\.mjs/.test(c) || /::error::This PR adds no changeset/.test(c)); + .filter((c) => /node scripts\/check-\S+\.mjs/.test(c) || /\bexit 1\b/.test(c)); assert( - chunks.length === 4, - `consumer: expected 4 changeset-verdict steps in the Check Changeset job, found ${chunks.length} -- a new one that this rule cannot see is a new way to red an exempt PR`, + chunks.length === 5, + `consumer: expected 5 failable steps in the Check Changeset job, found ${chunks.length} -- a new one that this rule cannot see is a new way to red an exempt PR`, + ); + // The step whose relocation #6434 IS. Pinned in the negative as well as the + // positive: base resolution reports into an output and the verdict is taken + // downstream, so re-introducing an `exit 1` here would restore the fast-path + // -only failure this card exists to remove. The count above would catch that + // as a 6th failable step; this says which one and why, so the next reader + // gets the reason and not just an arithmetic mismatch. + const diffbaseStep = jobText + .split(/\n(?= - name: )/) + .map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n')) + .find((c) => /- name: Resolve the diff base/.test(c)); + assert( + diffbaseStep !== undefined && !/\bexit 1\b/.test(diffbaseStep), + 'consumer: `Resolve the diff base` must not fail on the spot -- it runs BEFORE the settling read, so its verdict would be taken on the fast path alone and would red a PR whose skip-changeset label was still in flight (#6434). It reports `base_error` and the adjudication step below decides.', + ); + assert( + diffbaseStep !== undefined && /base_error=/.test(diffbaseStep), + 'consumer: `Resolve the diff base` must report an unusable base as a `base_error` output -- dropping it silently would leave the downstream adjudication permanently un-triggerable, i.e. a gate that passes because it never runs (#4690)', ); const unguarded = chunks.filter((c) => !/steps\.labels_settled\.outputs\.skip != 'true'/.test(c)); assert( @@ -778,6 +828,21 @@ function selfTest() { /::error::This PR adds no changeset[\s\S]{0,900}?\n\s+exit 1\n/.test(yaml), 'consumer: the "no changeset" verdict must still exit 1 -- #6378 removes a structural FALSE red, it does not relax the gate', ); + // The same constraint for #6434, and the reason it is written as a POSITIVE + // is the asymmetry that makes the negative one above worthless on its own: + // "the diff base no longer reds an exempt PR" is satisfied just as well by a + // step that stopped running, or by an adjudication whose `if:` can never be + // true. So the exempt direction is not asserted at all -- what is asserted + // is that the enforcing direction survived, in the one place it now lives. + const adjudication = jobText + .split(/\n(?= - name: )/) + .map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n')) + .find((c) => /- name: Require a usable diff base/.test(c)); + assert(adjudication !== undefined, 'consumer: the unusable-diff-base verdict step must exist (#6434) -- without it `base_error` is written and never read, which is a gate deleted rather than relocated'); + assert( + /steps\.diffbase\.outputs\.base_error != ''/.test(adjudication ?? '') && /\n\s+exit 1\n/.test(adjudication ?? ''), + "consumer: the unusable-diff-base verdict must fire on `base_error != ''` and exit 1 -- #6434 relocates a failure past the settling read, it does not forgive one. A PR the label reads did not exempt is still failed over a base that could not be resolved (#4690).", + ); assert( !/continue-on-error/.test(yaml), 'consumer: no step in pr-automation.yml may carry `continue-on-error` -- that would turn this gate into a warning, which is the one outcome #6378 rules out',