Skip to content

Commit 3d64288

Browse files
hotlongclaude
andauthored
fix(ci): 无法解析的 diff 基准改为落成 output,由已认两次标签读取的步骤裁决 (#6434) (#6508)
`changeset-check` 里能让 PR 变红的步骤共五个。PR #6429#6378)让其中四个同时 认两次活标签读取(快路径读 + 结算读),第五个 `Resolve the diff base` 只带快路径 守卫,却自带两处 `exit 1`(事件无 base 分支;`git merge-base` 算不出来)。它跑在 结算读之前,因此一个在窗口内(实测 +10..45s,正是常态)才落地的 `skip-changeset` 标签对它不可见 —— 标签晚到叠加 git 基准真的失败时,它会在结算读有机会纠正之前 把一个本该豁免的 PR 判红。 本次采路线 3:该步骤只「报告」不「裁决」。两处 `exit 1` 改为写 `base_error` output 并 `exit 0`,裁决下移到新增的 `Require a usable diff base` —— 该步骤同时 认两次读取,是全 job 中唯一见过结算后标签状态的位置。 门禁没有被放宽:基准不可用对任何未被两次读取豁免的 PR 仍然 exit 1,#4690 管的是 判决本身而不是判决的位置。计数步骤与三个工具链步骤同步加上 `base_error == ''` 守卫 —— 对计数步骤是正确性(空 `$MERGE_BASE` 下 `git diff` 失败在管道中段,末端 `tr` 仍返回 0,该步会报出伪造的 `added=0`,把判决引向完全错误的错误信息),对工具 链步骤是成本与可读性(该路径上唯一的红只会是裁决步骤本身)。结算读的触发条件加一个 析取项,是同一条不变式而非放宽:等待仍只向「将红的 PR」收取,基准不可用同样是将红。 CONSUMER 断言的边界随之外扩并写明理由:判定步骤集合由「跑 check-*.mjs 或发出 no-changeset 错误」改为「跑 check-*.mjs 或含非注释的 exit 1」,4 → 5 条,并新增 「`Resolve the diff base` 不得就地失败」「裁决步骤必须存在且仍 exit 1」。#6434 的 缺口正是因为旧谓词看不见一个它没被告知的可失败步骤才存在,新谓词把这一类连带收进来。 43 → 48 条断言;三次消融(整文件回退 / 仅回退该步骤 / 仅删除裁决步骤)全部转红。 Claude-Session: https://claude.ai/code/session_01BDmDsu2575gDxeMCxXhDE3 Co-authored-by: Claude <noreply@anthropic.com>
1 parent c804f19 commit 3d64288

2 files changed

Lines changed: 170 additions & 19 deletions

File tree

.github/workflows/pr-automation.yml

Lines changed: 96 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,23 @@ jobs:
274274
# someone gives the checkout a `ref:`, where parent^1 becomes the PR's
275275
# previous commit. `merge-base` is right under BOTH checkouts, which is
276276
# why it is the one written here.
277+
#
278+
# #6434: this step REPORTS an unusable base, it does not adjudicate one.
279+
# Both of its former `exit 1`s are now `base_error` outputs, and the verdict
280+
# moved to `Require a usable diff base` further down -- the one place that
281+
# has seen BOTH label reads. The reason is the same #5580/#6378 race one
282+
# defect along: this step runs before the settling read, so a
283+
# `skip-changeset` label that lands in its window (the COMMON case, ~+10..45s
284+
# from PR creation) was still invisible here. Failing on the spot therefore
285+
# reddened a PR the job was about to exempt -- two conditions at once, the
286+
# late label plus a genuine git-base failure, which is why no instance was
287+
# ever observed and why the gap was recorded rather than guessed at.
288+
#
289+
# What is deliberately NOT changed: an unusable base is still a FAILURE for
290+
# every PR the changeset gate applies to. #4690 governs the verdict, not its
291+
# address. The warning below is emitted where the fact is discovered so the
292+
# infrastructure flake stays visible in the log even on the exempt path,
293+
# where nothing else would ever mention it.
277294
- name: Resolve the diff base (merge base with the base branch)
278295
id: diffbase
279296
if: steps.labels.outputs.skip != 'true'
@@ -282,8 +299,9 @@ jobs:
282299
PINNED_BASE_SHA: ${{ github.event.pull_request.base.sha }}
283300
run: |
284301
if [ -z "$BASE_REF" ]; then
285-
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)."
286-
exit 1
302+
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."
303+
echo 'base_error=This event carries no base branch, so the changeset diff base cannot be computed.' >> "$GITHUB_OUTPUT"
304+
exit 0
287305
fi
288306
if ! git rev-parse --verify --quiet "refs/remotes/origin/$BASE_REF^{commit}" >/dev/null; then
289307
git fetch --no-tags --quiet origin "+refs/heads/$BASE_REF:refs/remotes/origin/$BASE_REF" \
@@ -292,11 +310,17 @@ jobs:
292310
# `if !` rather than a bare assignment on purpose: these steps run under
293311
# `bash -e` (no `shell:` key anywhere in this file), where a failing
294312
# command substitution kills the step with no message at all. The gate
295-
# is allowed to fail here -- it is NOT allowed to fail unexplained.
313+
# is allowed to fail over this -- it is NOT allowed to fail unexplained.
296314
if ! MERGE_BASE=$(git merge-base "refs/remotes/origin/$BASE_REF" HEAD); then
297-
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."
298-
exit 1
315+
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."
316+
echo "base_error=Could not compute merge-base(origin/$BASE_REF, HEAD), so the changeset diff has no trustworthy starting point." >> "$GITHUB_OUTPUT"
317+
exit 0
299318
fi
319+
# Written explicitly rather than left unset. An unwritten output already
320+
# reads as the empty string, but every downstream `if:` here spells the
321+
# success case as `base_error == ''`, and a contract that load-bearing
322+
# should be visible in the shell that establishes it.
323+
echo 'base_error=' >> "$GITHUB_OUTPUT"
300324
echo "merge_base=$MERGE_BASE" >> "$GITHUB_OUTPUT"
301325
# The drift is printed, not just corrected. #6129 was invisible for as
302326
# long as it was because nothing in the log ever said which commit the
@@ -306,18 +330,37 @@ jobs:
306330
echo "Diff base: $MERGE_BASE (merge-base of origin/$BASE_REF and HEAD)"
307331
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."
308332
333+
# The three toolchain steps and the counting step below all carry the
334+
# `base_error == ''` conjunct as well (#6434). For the counting step it is
335+
# CORRECTNESS: handed an empty `$MERGE_BASE`, `git diff --diff-filter=A ""
336+
# HEAD` fails, but it fails inside a pipeline whose last command is `tr`, so
337+
# the step would report `added=0` -- a fabricated count that reads exactly
338+
# like "this PR forgot its changeset" and would send the verdict step to the
339+
# wrong error message entirely. For the three toolchain steps it is COST and
340+
# legibility: an unusable base can no longer be adjudicated until the label
341+
# window settles, and there is no reason to buy a Node toolchain and a
342+
# `--frozen-lockfile` install first for a run that will either be exempted or
343+
# failed without ever compiling anything. It also keeps the failure signature
344+
# single: on this path the only red is the adjudication step, never an
345+
# install that happened to flake on the same shaky network.
309346
- name: Setup Node.js
310-
if: steps.labels.outputs.skip != 'true'
347+
if: >-
348+
steps.labels.outputs.skip != 'true'
349+
&& steps.diffbase.outputs.base_error == ''
311350
uses: actions/setup-node@v7
312351
with:
313352
node-version: '22'
314353

315354
- name: Enable Corepack
316-
if: steps.labels.outputs.skip != 'true'
355+
if: >-
356+
steps.labels.outputs.skip != 'true'
357+
&& steps.diffbase.outputs.base_error == ''
317358
run: corepack enable
318359

319360
- name: Install dependencies
320-
if: steps.labels.outputs.skip != 'true'
361+
if: >-
362+
steps.labels.outputs.skip != 'true'
363+
&& steps.diffbase.outputs.base_error == ''
321364
run: pnpm install --frozen-lockfile
322365

323366
# COUNTING ONLY -- the verdict is two steps down (#6378). The split is not
@@ -327,7 +370,9 @@ jobs:
327370
# that fact before any waiting is considered.
328371
- name: Count the changesets this PR adds
329372
id: changeset_count
330-
if: steps.labels.outputs.skip != 'true'
373+
if: >-
374+
steps.labels.outputs.skip != 'true'
375+
&& steps.diffbase.outputs.base_error == ''
331376
env:
332377
MERGE_BASE: ${{ steps.diffbase.outputs.merge_base }}
333378
run: |
@@ -405,11 +450,21 @@ jobs:
405450
# already behind us, and the loop does exactly one read and no sleeping.
406451
# Only a genuinely fresh PR can wait at all. 120s is ~3x the worst label
407452
# latency measured on this repo (~40s, #6358).
453+
#
454+
# #6434 adds the second disjunct, and it is the SAME invariant rather than a
455+
# widening of it: "charge the wait only to a PR that is headed for red". A PR
456+
# whose diff base could not be resolved is headed for red just as surely as
457+
# one that added no changeset -- it simply gets there by the other route, and
458+
# the counting step is skipped on that path, so `added` is `''` and the first
459+
# disjunct can never speak for it. No PR waits that would not have waited
460+
# under #6378: a resolvable base plus a changeset still skips this step
461+
# entirely, at no wall time and no API call.
408462
- name: Settle the skip-changeset window (only when this PR would otherwise fail)
409463
id: labels_settled
410464
if: >-
411465
steps.labels.outputs.skip != 'true'
412-
&& steps.changeset_count.outputs.added == '0'
466+
&& (steps.changeset_count.outputs.added == '0'
467+
|| steps.diffbase.outputs.base_error != '')
413468
env:
414469
GH_TOKEN: ${{ github.token }}
415470
PR_NUMBER: ${{ github.event.pull_request.number }}
@@ -464,6 +519,37 @@ jobs:
464519
echo "Label window closed after $ATTEMPT read(s). Labels on PR #$PR_NUMBER right now: ${LABELS:-(none)}"
465520
echo 'skip=false' >> "$GITHUB_OUTPUT"
466521
522+
# #6434: the verdict on an unusable diff base. It is a SEPARATE step rather
523+
# than a branch inside the step below, for the reason this job already gives
524+
# for splitting `Reject an empty-frontmatter changeset` off: the two failures
525+
# are different facts and deserve different messages. "The git base is
526+
# unavailable" is not "you forgot a changeset", and an author who reads the
527+
# second when the first is true will go looking in the wrong place.
528+
#
529+
# It sits HERE, immediately after the settling read, because both of its
530+
# neighbours constrain it. It cannot move up: above the settling read there
531+
# is no settled label state, which is the whole defect being fixed. It cannot
532+
# move down: the step below evaluates `[ "$ADDED" -eq 0 ]` on an `ADDED` that
533+
# is the empty string whenever the counting step was skipped, and `test`
534+
# answering "not a number" is a NON-zero status, so the `if` would take its
535+
# else branch and announce "This PR adds changeset(s)" -- a green verdict on
536+
# a gate that never ran. The ordering is load-bearing in both directions.
537+
#
538+
# This is NOT the route-2 move #6378's cost argument rules out. The settling
539+
# read stays exactly where it is, after the count; nothing about which PRs
540+
# pay for the window changed. What moved is one verdict, downward, past a
541+
# read that had already been paid for.
542+
- name: Require a usable diff base (adjudicated after the label window)
543+
if: >-
544+
steps.labels.outputs.skip != 'true'
545+
&& steps.labels_settled.outputs.skip != 'true'
546+
&& steps.diffbase.outputs.base_error != ''
547+
env:
548+
BASE_ERROR: ${{ steps.diffbase.outputs.base_error }}
549+
run: |
550+
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."
551+
exit 1
552+
467553
# The VERDICT. Everything it needs was decided above; this step only
468554
# announces it, which is what makes the failure message a single block of
469555
# prose rather than something interleaved with counting and polling.

scripts/check-empty-changeset.mjs

Lines changed: 74 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,20 @@ function selfTest() {
733733
settle,
734734
'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',
735735
);
736+
// The condition above is a substring test, so it would go on passing if a
737+
// later edit bolted an unrelated `|| <anything>` onto it and quietly taxed
738+
// every run again. #6434 legitimately adds ONE disjunct -- the unusable diff
739+
// base, which is the other way a PR arrives at red and which the count
740+
// cannot speak for, because the counting step is skipped on that path. So
741+
// the whole condition is pinned, not just its first term: exactly these two
742+
// ways in, and no third without an argument.
743+
const settleIf = yaml.match(
744+
/steps\.labels\.outputs\.skip != 'true'\s*\n\s*&& \(steps\.changeset_count\.outputs\.added == '0'\s*\n\s*\|\| steps\.diffbase\.outputs\.base_error != ''\)/,
745+
);
746+
assert(
747+
settleIf !== null,
748+
"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",
749+
);
736750

737751
// Both reads, one matcher. A divergence (say a substring `grep -q` on one
738752
// path) would be a gate that exempts on one read and enforces on the
@@ -744,20 +758,56 @@ function selfTest() {
744758
`consumer: exactly two live \`grep -qxF 'skip-changeset'\` reads are expected (the fast path and the settling read); found ${matchers.length}`,
745759
);
746760

747-
// Every step that can FAIL a PR over the changeset rule must honour both
748-
// reads. Scoped to those steps by what they run, not by name: a step that
749-
// shells out to a `check-*.mjs` gate, or that emits the "no changeset"
750-
// error. `Resolve the diff base` is deliberately outside this set -- it
751-
// exits 1 over an unusable git base, which is not a changeset verdict and
752-
// was never label-exempt (recorded, not implied).
761+
// Every step of this job that can FAIL a PR must honour both reads. Scoped
762+
// by what a step RUNS, never by its name: it shells out to a `check-*.mjs`
763+
// gate, or it contains a literal `exit 1` outside a comment.
764+
//
765+
// #6434 widened this boundary, and the widening is the point rather than an
766+
// accident of it. The predicate used to be "runs a `check-*.mjs` OR emits
767+
// the no-changeset error", which described the four steps that existed and
768+
// nothing else. `Resolve the diff base` sat outside it carrying two `exit
769+
// 1`s of its own -- deliberately, on the argument that "the git base is
770+
// unavailable" is not a changeset verdict and so was never label-exempt.
771+
// That argument is sound about the VERDICT and wrong about its ADDRESS: the
772+
// step ran before the settling read, so on a PR whose `skip-changeset` label
773+
// landed in the ordinary +10..45s window it could red a run the job was
774+
// about to exempt. #6434 moved that verdict to `Require a usable diff base`,
775+
// which honours both reads, and the boundary here moved with it.
776+
//
777+
// Naming `exit 1` instead of one specific error string is what makes the
778+
// rule outlive the steps it was written for: the gap #6434 closed existed
779+
// precisely because the old predicate could not see a failing step it had
780+
// not been told about, and the next one added here would have been invisible
781+
// the same way. Residual, stated rather than implied: a step that fails by
782+
// running a command that returns non-zero, with no literal `exit 1` and no
783+
// `check-*.mjs`, is still outside this set. That is a smaller hole than the
784+
// one it replaces, not no hole.
753785
const jobText = yaml.slice(yaml.indexOf('\n changeset-check:'));
754786
const chunks = jobText
755787
.split(/\n(?= - name: )/)
756788
.map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n'))
757-
.filter((c) => /node scripts\/check-\S+\.mjs/.test(c) || /::error::This PR adds no changeset/.test(c));
789+
.filter((c) => /node scripts\/check-\S+\.mjs/.test(c) || /\bexit 1\b/.test(c));
758790
assert(
759-
chunks.length === 4,
760-
`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`,
791+
chunks.length === 5,
792+
`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`,
793+
);
794+
// The step whose relocation #6434 IS. Pinned in the negative as well as the
795+
// positive: base resolution reports into an output and the verdict is taken
796+
// downstream, so re-introducing an `exit 1` here would restore the fast-path
797+
// -only failure this card exists to remove. The count above would catch that
798+
// as a 6th failable step; this says which one and why, so the next reader
799+
// gets the reason and not just an arithmetic mismatch.
800+
const diffbaseStep = jobText
801+
.split(/\n(?= - name: )/)
802+
.map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n'))
803+
.find((c) => /- name: Resolve the diff base/.test(c));
804+
assert(
805+
diffbaseStep !== undefined && !/\bexit 1\b/.test(diffbaseStep),
806+
'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.',
807+
);
808+
assert(
809+
diffbaseStep !== undefined && /base_error=/.test(diffbaseStep),
810+
'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)',
761811
);
762812
const unguarded = chunks.filter((c) => !/steps\.labels_settled\.outputs\.skip != 'true'/.test(c));
763813
assert(
@@ -778,6 +828,21 @@ function selfTest() {
778828
/::error::This PR adds no changeset[\s\S]{0,900}?\n\s+exit 1\n/.test(yaml),
779829
'consumer: the "no changeset" verdict must still exit 1 -- #6378 removes a structural FALSE red, it does not relax the gate',
780830
);
831+
// The same constraint for #6434, and the reason it is written as a POSITIVE
832+
// is the asymmetry that makes the negative one above worthless on its own:
833+
// "the diff base no longer reds an exempt PR" is satisfied just as well by a
834+
// step that stopped running, or by an adjudication whose `if:` can never be
835+
// true. So the exempt direction is not asserted at all -- what is asserted
836+
// is that the enforcing direction survived, in the one place it now lives.
837+
const adjudication = jobText
838+
.split(/\n(?= - name: )/)
839+
.map((c) => c.split('\n').filter((l) => !/^\s*#/.test(l)).join('\n'))
840+
.find((c) => /- name: Require a usable diff base/.test(c));
841+
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');
842+
assert(
843+
/steps\.diffbase\.outputs\.base_error != ''/.test(adjudication ?? '') && /\n\s+exit 1\n/.test(adjudication ?? ''),
844+
"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).",
845+
);
781846
assert(
782847
!/continue-on-error/.test(yaml),
783848
'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',

0 commit comments

Comments
 (0)