ci(timings): give the test-timing parser self-test teeth in lint.yml, not continue-on-error in ci.yml - #18096
ci(timings): give the test-timing parser self-test teeth in lint.yml, not continue-on-error in ci.yml#18096baozhoutao wants to merge 1 commit into
Conversation
… not continue-on-error in ci.yml `scripts/report-test-timings.mjs` ships a real self-test — 61 cases across 4 batteries, a naive prefix-only reference parser plus three controls that assert it gets the answer wrong exactly where the real parser gets it right. ci.yml named it, so `check:self-test-wired` was green. But the `Test Core` step that ran it carries `continue-on-error: true`, so its verdict was produced and then discarded: a broken timing parser stayed green, and the wiring gate was satisfied by an invocation that could not fail. That `continue-on-error` cannot simply be removed. #16454 rules the timing feature report-only and #14469 is the measured cost of an unguarded diagnostics step in that job (a FinalizeArtifact 403 on a 313-byte upload evicted a fully green shard from the merge queue). Removing it would also put `--capture`, which parses real and variable CI logs, on the shard's PASS/FAIL path. The guard that makes the feature safe is the same guard that disarmed its self-test, so the fix moves the instrument rather than weakening the guard. - lint.yml: new `Test-timing parser self-test` step in `Lint & Repo Gates`, unguarded, beside the shard-partitioner self-test it is the sibling of. - ci.yml: the `--self-test` invocation is removed from the capture step, and the paragraph that explained why it lived there is rewritten. `--capture` and `--merge` keep `continue-on-error`; that property is untouched. - report-test-timings.mjs: the docblock claimed report-only of "every workflow step that runs this". That is now false and was the sentence that made the capture guard read as a licence covering the self-test too. `check:self-test-wired` stays green with the invocation moved: lint.yml is a workflow, and the gate credits any workflow that names the script. Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU Co-authored-by: Claude <noreply@anthropic.com>
|
PM 复核:路线偏离成立,收下。 ⭐⭐ 1. 你没照字面执行的那一条,字面执行会违反树上写死的禁令本席的简报(以及分诊)写的是「把那一步的 前提一 —— 那一步不止跑 self-test( - name: Capture this shard's test timings
if: always()
continue-on-error: true
run: |
…
node scripts/report-test-timings.mjs --self-test ← :945
node scripts/report-test-timings.mjs --capture \ ← :946⇒ 摘掉那个 key,会把 ** 前提二 ——
⇒ ⭐⭐ 树自己既禁止了那个动作,又点名了正确的替代。 本席的简报要求的,是一件这棵树明文不许做的事;而你做的,正是它写下的那一支 —— 也是卡面 direction 1 里被列为首选的那一支。 ⇒ 偏离成立,而错的是本席的验收第 1 条。 ⭐ 一条「摘掉 tolerance」的指令,在一个 tolerance 同时罩着两样东西的步骤上是错的;正确的动作不是削弱守卫,而是把仪器挪到能执行判决的地方。 本席核过你挪的结果:
2. 其余各项
|
Closes #17097
Clause-②: no
The pinned first step: the self-test was GREEN before removal
Triage required this before anything else, and it decides the path:
Exit 0, on
origin/mainat66e34d14d9, before any edit. Socontinue-on-errorwas not masking a live failure, and this lands on the "passes today" path — no surprise red, and no tolerance re-added anywhere. (Card said ~57 cases; the battery is now 61. Same 4 batteries.)What changed, and why not the literal removal
The brief asked for
continue-on-errorto come off theCapture this shard's test timingsstep. I measured that first and did not do it — it would re-open a standing ruling with a measured cost, so this takes the card's own leading direction instead. Reporting the divergence plainly rather than choosing silently:--capture, which parses real and variable CI logs. Removing the key puts it on the shard's PASS/FAIL path.actions/upload-artifactFinalizeArtifact returned a non-retryable 403 on two shards at the same second, and the fail-closed attestation gate read them as missing #14469 is the measured cost of an unguarded diagnostics step in that exact job — a FinalizeArtifact 403 on a 313-byte upload evicted a fully green shard from the merge queue.ci.ymlalready carried the remedy in its own comment: "Giving that self-test teeth means acheck:step in lint.yml." That is direction 1 in the card body, named there as the leading candidate because it "keeps ci.yml's report-only property intact by putting the enforcement somewhere else entirely."The guard that makes the feature safe is the same guard that disarmed its self-test. So the fix moves the instrument rather than weakening the guard:
lint.ymlTest-timing parser self-teststep inLint & Repo Gates— nocontinue-on-error, noif:— beside the shard-partitioner self-test it is the sibling ofci.yml--self-testinvocation removed from the capture step; the paragraph explaining why it lived there rewritten.--capture/--mergekeepcontinue-on-error— untouchedreport-test-timings.mjsOne enforcing run, not one enforcing and one decorative.
check:self-test-wiredstays green with the invocation moved:lint.ymlis a workflow, and the gate credits any workflow that names the script.Acceptance 1 — the verdict can turn the job red
Read back out of the parsed YAML, not out of the diff:
The step is a single command, so the step's exit is the self-test's exit.
Acceptance 2 — positive control: it DOES go red
Ablation on the committed tree, degrading the real parser back to the naive shape the controls exist to catch —
TEST_FILEwidened to/./, so the project label is taken as the path (the wrongness CONTROL 3 pins).On-disk proof taken before reading any run — anchor and marker each counted, never a bare
git diff --stat:Exit 1 ⇒ the step fails ⇒
Lint & Repo Gatesgoes red. The tolerance is not merely removed; it has been seen to fire.Restore settled by blob identity, never by an exit code and never by
trapalone (#17875 — atrapin this container can silently not fire):No
dist/leg: the workflow step runs this script from source, so there is no build for a stale artifact to hide in.Acceptance 3 — negative control: still green, no new noise
One line, the same line the step printed under
ci.yml. Nothing new is emitted, andcheck:self-test-workflow-commandsis green, so nothing here can mint an annotation on a runner.Acceptance 4 — reverse-read, including the zeros
Probe shape stated, because the key/prose distinction is the whole point. Two independent probes:
grep -rnE '^[[:space:]]*continue-on-error[[:space:]]*:' .github/workflows/— matches only a line-initial YAML key.yaml.safe_loadand test whethercontinue-on-erroris a member of the workflow / job / step mapping. This cannot be fooled by prose at all.Both agree: 13 keys across 34 workflow files.
ci.ymlgoverned-surface-guard.ymlcheck-links.ymllint.ymldocs-drift-check.ymlrelease.ymlrerun-safety-nightly.ymlThe four bolded zero rows are exactly the difference the brief flagged: prose that mentions the key, in three cases specifically to say the step deliberately does not carry it. A textual search for the bare word reports 7 files; only 3 of them actually have one.
Is any other verdict being masked? Zero — after this PR. Classifying all 13 key sites by whether the step runs a verdict (
--self-test,check:*, or acheck-*.mjs):ci.yml→Capture this shard's test timings, this card.actions/upload-artifact/actions/download-artifact— artifact IO, no verdict.run:steps inci.yml(Capture…,Publish the test timing table) are the report-only timing path ci: every Test Core run publishes the slowest test files and packages beside their pinned weights (maintainer-directed, part B measurement) #16454 ruled.governed-surface-guard.ymlare pnpm setup/cache/install only. Its two verdict steps —Guard predicate self-testandGoverned surfaces may not enter the merge queue unreviewed— carry nocontinue-on-error. Correct by construction.check-links.yml's single key is attempt 1 of a retry pair; its own comment records that it hands the verdict to the second attempt.So this card was the only instance of its class in the workflow set, and the class is now empty.
Reverse-read of the diff — which existing sentence goes false?
Not a zero. One present-tense bare claim rotted, and it is fixed in this PR:
After this change the
lint.ymlstep runs it with neither. That sentence was also load-bearing in the wrong direction — it generalised aci.ymlproperty to the whole repo, which is precisely what made the capture guard read as covering the self-test too. Rewritten to scope the claim toci.ymland to name the unguardedlint.ymlleg.The
ci.ymlparagraph that said giving the self-test teeth is "deliberately NOT taken here" also went false; rewritten in place. No pin test anywhere in the tree fixed the old behaviour —git grep report-test-timingsoutsideci.ymlreturns only the script's own strings, so nothing had to be re-judged and nothing was deleted.Verification
node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 63 families from the real change set. 48 run, all green, exit codes captured before any pipe. Highlights:Declared narrowing — 14 families not run locally, handed to CI:
pnpm check:type-check-debt:: exit 3, PREREQUISITE NOT MET (wants a build). NOT MEASURED — recorded as neither pass nor finding.dts-closure,dual-build-cjs-loads,lean-entry-closure,sourcemap-no-sources-content,console-sha,cross-package-test-inputs,driver-memory-census,stall-guard-budget,stall-guard-headroom,type-check-coverage, and the rest) read package build outputs or package source. This diff is two workflow files plus one repo-root script that no package ships (nofiles[]in any manifest names it), so they are structurally unable to move. CI runs the full farm.Changeset — measured, then judged
skip-changeset, applied as a label. Nothing published moves: the diff is.github/workflows/**plusscripts/report-test-timings.mjs, and no package manifest'sfiles[]names that script or thescripts/directory. Rootpackage.jsondeclares nofiles[].Generated by Claude Code