Skip to content

fix(ci): collect every self-test verdict in a step, instead of masking after the first failure - #10887

Merged
os-zhuang merged 2 commits into
mainfrom
claude/issue-10814-shallow-history-step-split
Aug 21, 2026
Merged

fix(ci): collect every self-test verdict in a step, instead of masking after the first failure#10887
os-zhuang merged 2 commits into
mainfrom
claude/issue-10814-shallow-history-step-split

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Fixes#10814

GitHub runs a run: block as bash -e SCRIPT (a real shell, with errexit set), 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-testfirst, and that one was red on main for ~10 h on 2026-08-21 (#10807). For that whole window check-engine-split-ratio.mjs --self-test and collect-release-notes.sh --self-testdid 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.

Note: SCRIPT and GATE below are placeholders written as words on purpose. Spelled with angle brackets they are eaten by the body sanitizer, backticks included — this body lost four such fragments on its first write.

What changed

Both steps carrying 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.

scripts/check-step-collectors.mjs (new) closes the class in two halves:

  • Static — a run: block invoking --self-test on two or more distinct scripts must route them through a collector. Deliberately narrow: it does not flag the pair GATE --self-test then GATE, this job's dominant shape, where the abort is the point (a checker whose own self-test failed has no verdict worth printing), nor ci.yml's mkdir -p / psql ALTER SYSTEM dependency sequences.
  • Dynamic — the load-bearing half, because nothing static can tell a collector that propagates the exit code from one that swallows it, and the swallowing kind is worse than the masking it replaces: a green step over a red self-test is indistinguishable from success. --self-test extracts each live block out of lint.yml, writes it to a file, and runs it as bash -e SCRIPT — the invocation Actions uses — against stubs with controlled exit codes, reading "did this command run" from the stub's own side effect, never from the block's output. The block cannot vouch for itself.

Evidence — predictions stated before running, at 05e3b98

node scripts/check-step-collectors.mjs --self-test59 assertions, 2 live blocks driven under a real errexit shell. Measured, for each of the two blocks (3 self-tests each):

caseexitstubs executedverdicts printed
all pass03/33 PASS
#1 fails (the #10807 position)13/31 FAIL + 2 PASS, summary names #1
#2 fails13/31 FAIL + 2 PASS, summary names #2
#3 fails13/31 FAIL + 2 PASS, summary names #3
all fail13/33 FAIL, summary names all three

Both directions the card asked for: a failure in any one self-test no longer hides the others' verdicts, and the step still fails overall.

Ablation — the pre-fix shape, same command list, identical harness:

caseexitstubs executed
pre-fix bare sequence, #1 fails11/3 ← the defect, reproduced
pre-fix bare sequence, all pass03/3 ← so the mask is caused by the failure, not by the harness

A harness that cannot reproduce the defect cannot certify the fix, so that pair is pinned in the self-test too.

The census

Derived, not recalled: 343 run: steps across 26 workflows. Restricting to lint.yml + ci.yml, 21 of 200 steps hold two or more substantive commands (16 straight-line, 5 with control flow). Of all 343, exactly two sequenced two-or-more distinct self-tests — the two fixed here, both in lint.yml's lint job:

  • Shallow-history guard self-tests — the card's step.
  • scripts/ shared-module self-tests (parse · entry predicate · comment mask) — the same defect, found by the census. ts-parse sits first, is the slowest of the three and the only one that spawns real children, so it is the likeliest to be red: precisely the masking direction.

Every other multi-command block is a precondition (GATE --self-test then GATE; a node -e timezone assert then the run) or a dependency (mkdir -p "$RUNNER_TEMP/…", the psql ALTER SYSTEM / pg_reload_conf pair), where abort-on-first-failure is the correct semantics. No pattern beyond these two, so no follow-up card.

Why collect, and not split into three steps

⛔ A plain step split does not fix this at all — 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 (the reason the required-context pin step carries none).

The card's mandatory check, done anyway, and both gates would have tolerated a split — so this is a choice on the merits, not a constraint:

  • check-shard-attestationscanWorkflow reads join(root, '.github', 'workflows', 'ci.yml') and nothing else. lint.yml is never opened, so no step edit here is visible to it. Its classification is also per step (stepsOf(job).some(...)), never over a job's joined text (check-shard-attestation.mjs misclassifies a shard job as an aggregate gate on a bare --verify substring anywhere in its run: text (false red, measured) #6589), so even in ci.yml a split cannot move a job across the gate/attester boundary.
  • check-required-contexts — asserts job-level properties (name:, no strategy.matrix, no truthy continue-on-error, merge_group:/pull_request: triggers) plus one step-level wiring assertion, which targets the pnpm check:required-contexts step specifically. Neither is reachable from the shallow-history step's structure.

Both re-run green against the edited file: ✓ check-shard-attestation: 2 aggregate gate(s) count 3 declared leg(s) across 3 attesting job(s). and ✓ check-required-contexts: 6 required context name(s) pinned across 2 workflow(s); 5 instruction surface(s) scanned against 2 retired name(s) (#9491).

Gates run — union at 05e3b98, each quoting its own verdict line

Derived with node scripts/pm/dispatch-gates.mjs (15 families) and re-derived unchanged after each commit; 14 of 15 run green, plus check:nul-bytes. Re-run in full at 05e3b98, all green:

✓ check-nul-bytes --self-test: 75 assertions over a temp git repo (real scan() path)
✓ check:entry-guard: 130 scripts/ file(s) — every entry guard goes through invoked-as.mjs; 88 export bindings, 78 of them inert on import
✓ check:parse-guard: 129 scripts/ file(s) — every TypeScript parse goes through ts-parse.mjs.
✓ check-required-contexts: 6 required context name(s) pinned across 2 workflow(s)
✓ check-shard-attestation: 2 aggregate gate(s) count 3 declared leg(s) across 3 attesting job(s).
✓ check-aggregator-roster: 3 aggregator(s) across 2 workflow(s); roster == needs: in both directions
✓ check-workflow-status-functions --self-test: 34 assertions over temp fixture roots (real scan() path)
✓ check:type-check-coverage --self-test — 34 semantic + 24 observation + 25 re-measure + 28 built-closure + 19 auto-lowering case(s) hold.
check-node-version: OK (32 setup-node step(s) across 26 workflow(s), all on Node 22).
OK: 13 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
OK: all 82 declared cross-package glob(s) (71 unique) are covered by `core` or `crosspkg`
✓ check-step-collectors --self-test: 59 assertions, 2 live block(s) driven under a real `bash -e`.
✓ check-step-collectors: 344 `run:` steps across 26 workflow(s); 2 step(s) run 2+ independent self-tests, all of them through a collector.

⚠️One declared narrowing:pnpm check:type-check-debt was not run. It refuses to measure rather than measuring a different world (#6376) — "--re-measure cannot run: 55 workspace dependenc(ies) of the ledgered packages have no built type entry point on disk" — and clearing that refusal needs a full cold turbo run build of every package in a shared container. This diff contains zero TypeScript (one workflow file, one .mjs under scripts/), and the wiring half of the same script, which is what reads lint.yml, ran green above. CI builds the closure before that step and measures it properly.

⚠️Serial note for the reviewer: the dispatch said no open PR touches lint.yml; re-checking immediately before the push found #10777 (open, draft) does. Its hunk is at ~line 1288, disjoint from these edits (~187 and ~600), and the step it adds — node scripts/pr-labels.mjs --self-test — is a single self-test, which the new gate does not flag. No conflict expected in either direction, but two branches are in flight on this file.

Correction in 05e3b98 (PM review)

The second commit is comments only — no executable line changed, verified by filtering the diff to non-comment lines.

Both places where I explained the direct node scripts/… invocation claimed root package.json is "declared territory of the @changesets/cli v3 migration lane (#9465)". That reading is over-broad: the fence scopes to that file's @changesets/cli range and its version script, not the file, so a check:step-collectors key would have been allowed. The decision is unchanged and stands on its own precedent — lint.yml already invokes several gates directly and dispatch-gates derives families from either spelling — but the stated reason was wrong, and a comment asserting an unverified constraint reads exactly like a verified one and propagates to the next reader. That is this PR's own defect class one level up, so the accurate scope is now stated positively beside the decision rather than deleted. Only the two comments I authored were touched; the seven pre-existing instances of the same phrasing elsewhere in lint.yml are left alone as out of scope.

skip-changeset: this PR is workflow + scripts/ only and publishes nothing.


Generated by Claude Code


Generated by Claude Code

…g 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
@os-zhuangos-zhuang added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Aug 21, 2026 — with Claude
…r-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
@github-actions

Copy link
Copy Markdown
Contributor

⛔ merge queue 构建失败 — 先分诊,再决定要不要重排

队列构建 32501894648 红了。队列跑的是全量套件(PR 侧 CI 只跑 affected 子集),
所以失败的测试可能在本 PR 没碰过的包里 —— 那不是重排能修的。每次盲目重排都会让排在后面的所有 PR 重建一轮。

失败的 job(日志抽取,best effort):

  • Console Pin Gate — 失败步骤: Build the Console SPA at the pinned objectui SHA

    ✗ Build failed in 6.52s
    

↳ 失败原因 是判读的关键:超时Test timed out in … / Hook timed out in …)多半是负载/时序,不是本 PR 的回归;
断言AssertionError: …)才指向真实的行为改变。两者的 FAIL 行长得一模一样,只有这一行能区分。

跨 PR 相同签名(24h,按失败测试文件聚合):

  • ⚠️本次没有可用的聚合签名(日志里没有能解析出测试文件名的 FAIL 行)—— 这不是「没有同签名的其他 PR」,是这一轮没测到。跨 PR 聚合本次不可用,请手工比对其他 PR 的同类评论。
  • ⚠️ 24h 评论账本没读完(超过 5 页仍未读到窗口尽头),所以上面的「不同 PR 数」是下界,不是全量。

历史信号:

  • 本 PR 过去 24h 无队列失败记录(首次)。
  • 过去 24h 队列共有 32 个失败构建(不含本次)。

分诊清单:

  1. 失败测试在本 PR 改动的包里 → 真回归,修 PR。
  2. 失败测试与本 PR 无关 → 看上面的「跨 PR 相同签名」;已有汇总 issue ⇒ flaky/环境问题实锤,去那张 issue 上谈,修好前重排只会再烧一轮全队列。
  3. 两者都不是 → 可能与同组 PR 语义冲突;等前面的 PR 落地或失败出队后再重排一次即可,不要连续重排。

Generated by Claude Code · merge-queue-triage workflow (#4859)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/cdsize/lskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@os-zhuang@claude