Skip to content

Commit ee8f7c3

Browse files
os-zhuangclaude
andauthored
fix(ci): collect every self-test verdict in a step, instead of masking after the first failure (#10887)
* fix(ci): collect every self-test verdict in a step, instead of masking after the first failure GitHub runs a `run:` block as `bash -e <file>`, so the first non-zero exit aborts the step and every command after it is never reached -- neither green nor red, and nothing in the log tells those two apart. Measured, not hypothetical: lint.yml's `Shallow-history guard self-tests` ran three independent self-tests as a bare sequence with `git-history.mjs --self-test` first, and that one was red on `main` for about ten hours on 2026-08-21 (#10807). For the whole of that window `check-engine-split-ratio.mjs --self-test` and `collect-release-notes.sh --self-test` did not execute in CI once, on the step that gates every PR. Both were green, so the mask cost nothing that day; the expensive shape is the one it leaves behind, where a second regression lands unnoticed while the first is red and then reads as though the fix broke it. Both steps that had this shape now run every self-test unconditionally through a `run_self_test` collector, print a verdict per self-test, and exit non-zero at the end naming every one that failed. Not a split into one step per self-test: a plain split does not fix this at all, because Actions skips a job's remaining steps once a step fails, so the mask survives the split verbatim. Restoring the property would take an `if:` on each gate step, and a condition is a way for a PR to arrange that a gate does not run on it. Both gates that read step structure were checked and would have tolerated a split -- `check-shard-attestation` scans ci.yml only, and `check-required-contexts` pins job-level properties plus the single `check:required-contexts` step -- so the split was rejected on the merits, not because a gate refused it. `scripts/check-step-collectors.mjs` closes the class in both halves. Statically it requires any `run:` block invoking `--self-test` on two or more DISTINCT scripts to route them through a collector, and deliberately does not flag the `<gate> --self-test` + `<gate>` precondition shape that dominates this job. Dynamically -- the load-bearing half, since nothing static can tell a collector that propagates the exit code from one that swallows it -- its `--self-test` extracts each live block out of lint.yml and runs it as `bash -e <file>` against stubs with controlled exit codes, reading "did this command run" from the stub's own side effect rather than from the block's output. The same command list is also driven through the pre-fix bare sequence, which must mask. Census behind the narrow rule: 343 `run:` steps across 26 workflows, of which exactly two carried two or more distinct self-tests, both in lint.yml's `lint` job and both fixed here. Every other multi-command block is a precondition or a dependency sequence, where abort-on-first-failure is the correct semantics. Part of #10814 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt * docs(ci): rest the direct-node invocation on precedent, not on an over-broad #9465 fence The `Step-collector gate` step comment and the script docblock both claimed root `package.json` is "declared territory of the @changesets/cli v3 migration lane (#9465)". That reading is over-broad. The fence's own text scopes it to that file's `@changesets/cli` range and its `version` script -- the parenthetical in the issue body is scoping, not illustrative -- so the file itself is not fenced and a `check:step-collectors` key would have been allowed. The decision is unchanged and still correct: lint.yml already invokes several gates as `node scripts/...` directly, and dispatch-gates.mjs derives gate families from either spelling, so the direct form loses no discovery and adds no key to the root manifest. Only the stated REASON was wrong. Worth more than a silent reword, because it is this PR's own defect class one level up: a comment asserting a constraint nobody verified reads exactly like a verified one, and propagates to the next agent who reads it -- the same way a stale knownGap turns into a stale permission. So the accurate scope is now stated positively next to the decision rather than merely deleted. Only the two comments I authored are touched; the seven pre-existing instances of the same phrasing elsewhere in lint.yml are left alone as out of scope. Part of #10814 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2695a8c commit ee8f7c3

2 files changed

Lines changed: 642 additions & 6 deletions

File tree

‎.github/workflows/lint.yml‎

Lines changed: 145 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -184,11 +184,45 @@ jobs:
184184
# `ts-parse` spawns ~20 real node children (~10 s measured, and the spawns
185185
# are the point — they pin that a caller's try/catch cannot swallow the
186186
# refusal); the other two are in-process fixtures, ~0.5 s combined.
187+
#
188+
# ⭐ Collected rather than sequenced, for the reason spelled out at the
189+
# `Shallow-history guard self-tests` step below (#10814): under `bash -e` a
190+
# bare sequence stops at the first failure, so a red `ts-parse` would leave
191+
# the entry-predicate and comment-mask self-tests UNRUN while the log shows
192+
# only the one failure. `ts-parse` is both the slowest of the three and the
193+
# one that spawns real children, so it is the likeliest to be red —
194+
# precisely the masking direction. The three modules are independent of one
195+
# another, so collecting loses nothing.
187196
- name: scripts/ shared-module self-tests (parse · entry predicate · comment mask)
188197
run: |
189-
node scripts/ts-parse.mjs --self-test
190-
node scripts/invoked-as.mjs --self-test
191-
node scripts/js-comment-mask.mjs --self-test
198+
# Tolerate-and-collect (#10814) — see the note above this step. Each
199+
# self-test runs unconditionally and prints its own verdict; the step
200+
# still FAILS when any of them does, naming every one that failed.
201+
# ⛔ Never let the collector swallow the exit code — a green step over
202+
# a red self-test looks identical to success. Both directions are pinned
203+
# by `node scripts/check-step-collectors.mjs --self-test`, which extracts
204+
# THIS block from THIS file and drives it under `bash -e` with stubs.
205+
failed=""
206+
run_self_test() {
207+
echo "-- $*"
208+
if "$@"; then
209+
echo "PASS $*"
210+
else
211+
echo "FAIL $*"
212+
failed="${failed} $*"$'\n'
213+
fi
214+
return 0
215+
}
216+
run_self_test node scripts/ts-parse.mjs --self-test
217+
run_self_test node scripts/invoked-as.mjs --self-test
218+
run_self_test node scripts/js-comment-mask.mjs --self-test
219+
if [ -n "$failed" ]; then
220+
echo ""
221+
echo "scripts/ shared-module self-tests — the following FAILED:"
222+
printf "%s" "$failed"
223+
exit 1
224+
fi
225+
echo "scripts/ shared-module self-tests — all three ran and passed"
192226
193227
# Every `scripts/**` entry guard goes through ONE predicate (#10086).
194228
# The hand-typed forms of "did node run me, or did someone import me?"
@@ -527,11 +561,116 @@ jobs:
527561
# (`check-governed-merges.mjs`'s own cases run in the
528562
# `pnpm check:pm-governed-merges` step above, which is already its
529563
# self-test.)
564+
#
565+
# ⭐ The three run through a COLLECTOR rather than as a bare sequence
566+
# (#10814). A `run:` block is executed by `bash -e`, so the first non-zero
567+
# exit aborts the STEP and every command after it is never reached —
568+
# neither green nor red, and nothing in the log tells those apart. Not
569+
# hypothetical here: while `git-history.mjs --self-test` was red on `main`
570+
# for ~10 h on 2026-08-21 (#10807), the two self-tests listed after it did
571+
# not execute in CI once — on the step that gates every PR. Both were
572+
# green, so that time the mask hid nothing; the compounding shape is that a
573+
# SECOND regression can land unnoticed while the first is red, and then
574+
# reads as though the fix broke it. #4690 one level up: a partial result
575+
# that reads like a complete one.
576+
#
577+
# These three are INDEPENDENT — none is a precondition for reading the
578+
# next — which is what makes collecting correct HERE and
579+
# abort-on-first-failure correct in this job's many
580+
# `<gate> --self-test` + `<gate>` steps, where the self-test IS the
581+
# precondition for trusting the run after it. The census behind that
582+
# distinction: of 200 `run:` steps in lint.yml + ci.yml, 21 hold two or
583+
# more substantive commands, and exactly two were independent self-tests
584+
# sequenced together — this step and the `scripts/` shared-module step
585+
# above. Every other one is a precondition or a dependency, where the
586+
# abort is the correct semantics.
587+
#
588+
# ⛔ Not one step per self-test: a plain step split does not fix this at
589+
# all — Actions skips a job's remaining steps once a step fails, so the
590+
# mask survives the split verbatim. Restoring it would take an `if:` on
591+
# each gate step, and a condition is a way for a PR to arrange that a gate
592+
# does not run on it (the reason the required-context pin step carries
593+
# none). Both gates that read step structure were checked and would
594+
# TOLERATE a split — `check-shard-attestation` scans ci.yml only, and
595+
# `check-required-contexts` pins job-level properties plus the one
596+
# `check:required-contexts` step — so this is a choice on the merits,
597+
# not a constraint.
530598
- name: Shallow-history guard self-tests
531599
run: |
532-
node scripts/pm/git-history.mjs --self-test
533-
node scripts/check-engine-split-ratio.mjs --self-test
534-
bash scripts/collect-release-notes.sh --self-test
600+
# Tolerate-and-collect (#10814) — see the note above this step. Each
601+
# self-test runs unconditionally and prints its own verdict; the step
602+
# still FAILS when any of them does, naming every one that failed.
603+
# ⛔ Never let the collector swallow the exit code — a green step over
604+
# a red self-test looks identical to success. Both directions are pinned
605+
# by `node scripts/check-step-collectors.mjs --self-test`, which extracts
606+
# THIS block from THIS file and drives it under `bash -e` with stubs.
607+
failed=""
608+
run_self_test() {
609+
echo "-- $*"
610+
if "$@"; then
611+
echo "PASS $*"
612+
else
613+
echo "FAIL $*"
614+
failed="${failed} $*"$'\n'
615+
fi
616+
return 0
617+
}
618+
run_self_test node scripts/pm/git-history.mjs --self-test
619+
run_self_test node scripts/check-engine-split-ratio.mjs --self-test
620+
run_self_test bash scripts/collect-release-notes.sh --self-test
621+
if [ -n "$failed" ]; then
622+
echo ""
623+
echo "Shallow-history guard self-tests — the following FAILED:"
624+
printf "%s" "$failed"
625+
exit 1
626+
fi
627+
echo "Shallow-history guard self-tests — all three ran and passed"
628+
629+
# Step-collector gate (#10814) — the guard over the two steps above, and
630+
# over any step that grows their shape later. It has a static half and a
631+
# dynamic half, and the dynamic one is the load-bearing part:
632+
#
633+
# STATIC: one `run:` block invoking `--self-test` on TWO OR MORE DISTINCT
634+
# scripts must route them through a collector. Distinct scripts testing
635+
# themselves are independent by construction, so there is no reading under
636+
# which a failure in one should skip the others. Deliberately narrow: it
637+
# does NOT flag `<gate> --self-test` + `<gate>`, this job's dominant shape,
638+
# where the abort IS the point (a checker whose self-test failed has no
639+
# verdict worth printing), nor ci.yml's `mkdir -p` / `psql ALTER SYSTEM`
640+
# dependency sequences. Swept when it was written: 343 `run:` steps across
641+
# 26 workflows, exactly 2 matched, both of them above.
642+
#
643+
# DYNAMIC: nothing static can tell a collector that PROPAGATES the exit
644+
# code from one that swallows it, and the swallowing kind is worse than
645+
# the masking it replaces — a green step over a red self-test, which from
646+
# outside is indistinguishable from success. So `--self-test` extracts each
647+
# live block out of THIS file and runs it as `bash -e <file>` — the same
648+
# invocation Actions uses — against stubs with controlled exit codes, and
649+
# reads "did this command run" from the STUB's own side effect rather than
650+
# from the block's output, so the block cannot vouch for itself. Both
651+
# directions are pinned in every position, and the same command list is
652+
# also driven through the PRE-FIX bare sequence, which must mask — 1 of 3
653+
# commands executing when the first fails, 3 of 3 when none does. A harness
654+
# that cannot reproduce the defect cannot certify the fix.
655+
#
656+
# Invoked as `node scripts/…` rather than through a `pnpm check:*` alias,
657+
# on the precedent this job already sets: several gate steps here are
658+
# invoked directly, and dispatch-gates.mjs derives gate families from
659+
# either spelling, so the direct form loses no discovery and adds no key
660+
# to the root manifest.
661+
#
662+
# ⚠️ NOT because root package.json is off limits. The #9465 changeset lane
663+
# fences that file's `@changesets/cli` range and its `version` script — the
664+
# parenthetical in the issue body is scoping, not illustrative — and not
665+
# the file, so a `check:step-collectors` key would have been allowed. Said
666+
# plainly because the over-broad reading is easy to acquire and then
667+
# propagates as a constraint nobody actually has, which is this step's own
668+
# defect class wearing a different hat: a claim that reads as verified.
669+
# Temp-dir fixtures, no network, ~1 s.
670+
- name: Step-collector gate (self-tests that mask each other)
671+
run: |
672+
node scripts/check-step-collectors.mjs --self-test
673+
node scripts/check-step-collectors.mjs
535674
536675
# Verify-lock entry-point self-test (#9661). `scripts/pm/os-verify-lock.sh`
537676
# is the ONE way an agent takes the container's shared heavy-verify lock,

0 commit comments

Comments
 (0)