Skip to content

Commit 2a4a796

Browse files
baozhoutaoclaude
andauthored
ci(timings): give the test-timing parser self-test teeth in lint.yml, not continue-on-error in ci.yml (#18096)
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: ``` $ node scripts/report-test-timings.mjs --self-test report-test-timings: self-test OK (61 cases across 4 batteries — file-line parsing 22/22, attribution and the naive-parser controls 9/9, package seconds, slices and cache replays 11/11, refusals, rendering and exit codes 19/19) $ echo $? 0 ``` Exit **0**, on `origin/main` at `66e34d14d9`, before any edit. So `continue-on-error` was **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-error` to come off the `Capture this shard's test timings` step. **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: - That step also runs `--capture`, which parses **real and variable CI logs**. Removing the key puts it on the shard's PASS/FAIL path. - #16454 rules the timing feature **report-only**; #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.yml` already carried the remedy in its own comment: *"Giving that self-test teeth means a `check:` 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: | file | change | |---|---| | `lint.yml` | new **`Test-timing parser self-test`** step in `Lint & Repo Gates` — no `continue-on-error`, no `if:` — beside the shard-partitioner self-test it is the sibling of | | `ci.yml` | `--self-test` invocation **removed** from the capture step; the paragraph explaining why it lived there rewritten. `--capture` / `--merge` keep `continue-on-error` — untouched | | `report-test-timings.mjs` | docblock claimed report-only of *"every workflow step that runs this"* — now false, and it was the sentence that made the capture guard read as a licence covering the self-test | One enforcing run, not one enforcing and one decorative. `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. ## Acceptance 1 — the verdict can turn the job red Read back out of the parsed YAML, not out of the diff: ``` step 'Test-timing parser self-test' continue-on-error : ABSENT step 'Test-timing parser self-test' if : ABSENT job 'lint' continue-on-error : ABSENT workflow lint.yml continue-on-error : ABSENT lint.yml on.push / on.pull_request paths filter : none (runs on every PR) ``` 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_FILE` widened 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`: ``` HEAD blob : f861657 before : anchor=1 marker=0 after : anchor=0 marker=1 ← mutation reached disk mutated blob : 8d87ae409811b5341a3b9f2c742d9802eac951ec MUTATED_SELF_TEST_EXIT = 1 Error: split: a line with no test file: got {"file":"here","project":"no path"}, want null ``` **Exit 1 ⇒ the step fails ⇒ `Lint & Repo Gates` goes 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 `trap` alone (#17875 — a `trap` in this container can silently not fire): ``` restored blob : f861657 == HEAD blob git diff HEAD : empty ``` 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 ``` EXIT=0 stdout+stderr lines : 1 bytes : 230 annotation-shaped tokens (line-initial :: or ##[) : 0 ``` One line, the same line the step printed under `ci.yml`. Nothing new is emitted, and `check:self-test-workflow-commands` is 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: 1. **Key form, textual:** `grep -rnE '^[[:space:]]*continue-on-error[[:space:]]*:' .github/workflows/` — matches only a line-initial YAML key. 2. **Key form, structural:** parse every workflow with `yaml.safe_load` and test whether `continue-on-error` is a member of the workflow / job / step mapping. This cannot be fooled by prose at all. Both agree: **13 keys** across 34 workflow files. | workflow | keys | text mentions | |---|---|---| | `ci.yml` | 8 | 21 | | `governed-surface-guard.yml` | 4 | 6 | | `check-links.yml` | 1 | 3 | | **`lint.yml`** | **0** | **4** | | **`docs-drift-check.yml`** | **0** | **1** | | **`release.yml`** | **0** | **1** | | **`rerun-safety-nightly.yml`** | **0** | **1** | | every other workflow (27 files) | **0** | **0** | The 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 a `check-*.mjs`): - **0 of 13** now run a verdict. Before this PR the count was **1**: `ci.yml` → `Capture this shard's test timings`, this card. - 8 of the 13 are `actions/upload-artifact` / `actions/download-artifact` — artifact IO, no verdict. - The 2 remaining `run:` steps in `ci.yml` (`Capture…`, `Publish the test timing table`) are the report-only timing path #16454 ruled. - The 4 in `governed-surface-guard.yml` are pnpm setup/cache/install only. Its two verdict steps — `Guard predicate self-test` and `Governed surfaces may not enter the merge queue unreviewed` — carry **no** `continue-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: > `scripts/report-test-timings.mjs`: *"Every workflow step that runs this carries `if: always()` and `continue-on-error: true`"* After this change the `lint.yml` step runs it with neither. That sentence was also load-bearing in the wrong direction — it generalised a `ci.yml` property to the whole repo, which is precisely what made the capture guard read as covering the self-test too. Rewritten to scope the claim to `ci.yml` and to name the unguarded `lint.yml` leg. The `ci.yml` paragraph 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-timings` outside `ci.yml` returns 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/objectstack` derived **63** families from the real change set. **48 run, all green**, exit codes captured before any pipe. Highlights: ``` node scripts/check-self-test-wired.mjs :: exit 0 node scripts/check-self-test-wired.mjs --self-test :: exit 0 node scripts/check-self-test-workflow-commands.mjs :: exit 0 node scripts/check-step-collectors.mjs :: exit 0 node scripts/check-ci-filter-parity.mjs :: exit 0 node scripts/check-aggregator-roster.mjs :: exit 0 pnpm check:workflow-step-name-quoting :: exit 0 pnpm check:workflow-status-functions :: exit 0 pnpm check:required-contexts :: exit 0 pnpm check:shard-attestation :: exit 0 pnpm check:select-gate-families :: exit 0 pnpm check:pm-dispatch-gates :: exit 0 pnpm check:nul-bytes :: exit 0 pnpm check:pm-clause2-carriers :: exit 0 node scripts/report-test-timings.mjs --self-test :: exit 0 ``` **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. - 13 build-dependent families (`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** (no `files[]` 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/**` plus `scripts/report-test-timings.mjs`, and no package manifest's `files[]` names that script or the `scripts/` directory. Root `package.json` declares no `files[]`. --- _Generated by [Claude Code](https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent b57671e commit 2a4a796

3 files changed

Lines changed: 51 additions & 12 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -928,12 +928,17 @@ jobs:
928928
# here is fine; moving them below that pair is not — and ⛔ that pair does
929929
# not acquire an `if:` or a `continue-on-error:` to "match" these.
930930
#
931-
# The `--self-test` runs here because this is where CI names the script,
932-
# which is what check:self-test-wired requires. ⚠ Under
933-
# `continue-on-error` its verdict is VISIBLE but not enforcing: inside
934-
# this workflow, no step this card may add is permitted to fail anything.
935-
# Giving that self-test teeth means a `check:` step in lint.yml, which is
936-
# deliberately NOT taken here.
931+
# ⛔ The `--self-test` is deliberately NOT invoked here (#17097). It ran
932+
# in this step until then, where `continue-on-error` discarded its verdict:
933+
# the self-test reached an answer nothing could act on, and
934+
# `check:self-test-wired` was satisfied by an invocation that could not
935+
# fail. A self-test with no teeth is worse than no self-test, because its
936+
# presence is what stops anyone from adding a real one. It now runs in
937+
# lint.yml's `Lint & Repo Gates` job, as `Test-timing parser self-test`,
938+
# with no `continue-on-error` — so a broken timing parser reddens there
939+
# while this workflow keeps the report-only property #16454 ruled and
940+
# #14469 measured the cost of losing. ⛔ Do not re-add it here; giving it
941+
# teeth in `ci.yml` is exactly the trade-off those two cards refuse.
937942
- name: Capture this shard's test timings
938943
if: always()
939944
continue-on-error: true
@@ -942,7 +947,6 @@ jobs:
942947
echo "No test log — the test step did not get far enough to produce one."
943948
exit 0
944949
fi
945-
node scripts/report-test-timings.mjs --self-test
946950
node scripts/report-test-timings.mjs --capture \
947951
--log "$RUNNER_TEMP/test-core.log" \
948952
--summaries .turbo/runs \

‎.github/workflows/lint.yml‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3997,6 +3997,31 @@ jobs:
39973997
- name: Shard partitioner self-test
39983998
run: node scripts/partition-test-shards.mjs --self-test
39993999

4000+
# A self-test that RUNS but cannot fail anything (#17097) — the sibling of
4001+
# the defect above, one turn further out. `scripts/report-test-timings.mjs`
4002+
# ships a real self-test: 61 cases across 4 batteries, a naive prefix-only
4003+
# reference parser plus three controls that assert it gets the answer WRONG
4004+
# exactly where the real parser gets it right. ci.yml names it, so
4005+
# `check:self-test-wired` was green. But every `Test Core` step that runs
4006+
# that script carries `continue-on-error: true`, and MUST: #16454 rules the
4007+
# timing feature report-only, and #14469 is the measured cost of an
4008+
# unguarded diagnostics step there (a FinalizeArtifact 403 on a 313-byte
4009+
# upload evicted a fully green shard from the merge queue). So the guard
4010+
# that makes the feature safe was the same guard that disarmed its
4011+
# self-test — the verdict was produced and then discarded, and the wiring
4012+
# gate was satisfied by an invocation that could not fail.
4013+
#
4014+
# ⇒ the self-test runs HERE, where it has teeth, and ⛔ is no longer
4015+
# invoked from ci.yml: ONE enforcing run, not one enforcing and one
4016+
# decorative. ci.yml's own `continue-on-error` on the capture and merge
4017+
# steps is untouched by this — the report-only property belongs to that
4018+
# workflow, not to this script.
4019+
# Invoked as `node` rather than through a `pnpm check:*` alias for the same
4020+
# reason as the steps above: see the GATE INVOCATION IDIOM note at the top
4021+
# of this file. In-process fixtures, no IO, milliseconds.
4022+
- name: Test-timing parser self-test
4023+
run: node scripts/report-test-timings.mjs --self-test
4024+
40004025
# Hand-written `.d.mts` mirrors (#10549). `scripts/js-comment-mask.mjs`
40014026
# and `scripts/check-regen-pending.mjs` are untyped `.mjs` that each ship
40024027
# a hand-written declaration beside them, and both files say "keep this in

‎scripts/report-test-timings.mjs‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,21 @@
1818
* summaries (`.turbo/runs/*.json`, uploaded per shard) carry per-PACKAGE
1919
* execution windows and nothing finer.
2020
*
21-
* REPORT ONLY. Nothing here may redden `Test Core` -- that is the required
22-
* branch-protection context, and a diagnostics step that can fail it buys a
23-
* measurement at the price of the merge queue. Every workflow step that runs
24-
* this carries `if: always()` and `continue-on-error: true`, and the tool
25-
* itself degrades to a named refusal rather than throwing.
21+
* REPORT ONLY IN `ci.yml`. Nothing in that workflow may redden `Test Core` --
22+
* that is the required branch-protection context, and a diagnostics step that
23+
* can fail it buys a measurement at the price of the merge queue. Every
24+
* `ci.yml` step that runs this carries `if: always()` and
25+
* `continue-on-error: true`, and the tool itself degrades to a named refusal
26+
* rather than throwing.
27+
*
28+
* -- and that is a property of `ci.yml`, NOT of this script (#17097). This
29+
* docblock used to claim it of "every workflow step that runs this", which
30+
* made the `--capture`/`--merge` guard read as a licence covering the
31+
* self-test too. `--self-test` runs in lint.yml's `Lint & Repo Gates` job with
32+
* NO `continue-on-error`: a red verdict there is the whole point, and it is
33+
* the only instrument watching the parsing rules below -- a clean tree cannot
34+
* tell a working parser from a weakened one, which is what the naive-parser
35+
* controls in the battery exist to say out loud.
2636
*
2737
* ## THE MEASUREMENT THAT CHOSE THE ROUTE (route (a): parse the stream)
2838
*

0 commit comments

Comments
 (0)