refactor(i18n): run the unit-literal scan as a gate, not a vitest test - #2562
Merged
Merged
Conversation
The scan parses every in-scope file with the TypeScript compiler, so its cost scales with the size of the repo rather than with what a branch changed. Inside vitest that cost is multiplied by v8 coverage instrumentation and by every sibling worker. Measured on one 6-core box, same commit, only --maxWorkers varying: standalone gate .... 2.0 s 2 workers ......... 11.2 s 5.6x 4 workers ......... 13.7 s 6.8x <- GitHub CI shape, 15 s budget 6 workers ......... 16.6 s 8.2x 8 workers ......... 20.3 s 10.1x 12 workers ......... 28.6 s 14.2x The suite's 15 s per-test budget, sized for tests that `await import(...)`, is already marginal at the CI worker count and fails outright on a developer machine. Raising it only moves the number, because the cost tracks worker count. Throttling workers is worse: 12 -> 2 cuts the scan 60% but costs the whole suite 82% more wall clock (512 s -> 929 s). Even at 2 workers the scan pays a 5.6x tax it cannot escape, because v8 coverage is process-wide. There is no worker count at which this belongs in the suite. So it moves out, following the pattern this repo already uses for repo-scale gates (check-source-strings.mjs, check-i18n-strings.mjs, docs-lint.sh): - scripts/lib/unit-literals.mjs the matcher, shared by the gate and the tests - scripts/check-unit-literals.mjs the gate: ceiling + two diff-scoped checks - one line in the i18n gate table, which CI already runs; no workflow change - unitLiterals.test.ts keeps the four matcher tests (552 -> 75 lines, 17 ms) This is a refactor. The matcher and the diff-scoped logic are carried over unchanged, so the gate reports the same findings with the same attribution and the same limitations. Equivalence is verified rather than assumed: the old and new matchers agree file-by-file across every in-scope file, identical finding sets, nothing only-in-old and nothing only-in-new. Two behaviour notes, both called out in the issue rather than buried: The ceiling becomes `enforce: 'info'`. The gate table requires a whole-repo count that is not a hard zero to be informational, because one branch's inherited debt must not fail another branch's build. The two diff-scoped checks still stop any new site at zero tolerance. The diff-scoped markers print only when a base ref resolved. Printing them at zero with no base would claim a check that never ran, and would match the table's `find` regex and suppress its own NOT RUN branch.
Contributor
UX Review (Fable 5, fork) — ✅ PASSAdvisory UX-level review of This PR touches only CI tooling (a Node gate script, the gate-table registry, and tests) — no dashboard UI, user-facing strings, or rendered surfaces change. The only humans who "experience" it are contributors reading gate output, and those messages are checked below. Everything holds up; final review: UX-Verdict: PASS No product UI is touched; the developer-facing gate output states what failed, why it matters, and the exact fix — self-sufficient on a cold read. Suggestions
[UX-REVIEWED] 8362fe0 |
Contributor
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThis is a mechanical refactor: the number+unit literal matcher is extracted from I verified the key semantic points:
No findings. [OPUS-REVIEWED] 8362fe0 |
Contributor
Design Review (Fable 5, fork) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Right shape: repo-scale scan moves to the gate table where every peer already lives; enforcement changes are explicit and conform to the documented ratchet rule. Suggestions
[DESIGN-REVIEWED] 8362fe0 |
Contributor
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
iamwhatever
approved these changes
Aug 10, 2026
This was referenced Aug 10, 2026
kyleseaman
pushed a commit
that referenced
this pull request
Aug 10, 2026
The repo-wide scan parses every `.ts`/`.tsx` file under `src` -- ~1k files and ~12MB of TSX -- to find sites in a few dozen of them, and the parse is the overwhelming majority of its runtime. That cost is what made this scan too expensive to sit in the vitest suite at all: it timed out at 15650ms against the 15s per-test budget on main's Frontend Tests job (run 31359945669), which #2562 resolved by moving the scan out to a standalone gate. This removes the cost itself, so the gate that inherited it is ~2x cheaper and no longer scales with files it can never flag. A finding needs a literal chunk whose text starts with an optional space then a unit symbol, and in raw source such a chunk is always preceded by `}` (a template continuation, or JSX text after an expression) or by a quote directly after `+`. Reject a file on that raw-text test and it never gets parsed; 911 of 1026 files now don't. The unit vocabulary is read out of `UNIT` itself so the fast path cannot become narrower than the matcher, and the files where raw and cooked text can legitimately disagree -- an escape, or a comment between `+` and its operand -- are admitted unconditionally rather than reasoned about. Three smaller costs went with it: parent pointers are no longer requested from `createSourceFile` (the visitor passes down the ancestor chain it already knows), the CSS-context checks now run after the cheap unit test instead of before it, and the directory walk classifies entries from the `readdir` it already makes instead of a `stat` per entry. Measured on one box, interleaved A/B over 5 rounds, same corpus: scan phase (warm, in-process) .... 1439ms -> 434ms gate end-to-end (median) ......... 4313ms -> 2376ms The matcher is unchanged and still decides every finding. Verified equivalent against the previous implementation over all 1031 in-scope files: identical per-file hit lists, 52 findings before and after, and zero findings in any file the fast path skips. A new test pins the fast path to the matcher with a case per clause that is a real finding admitted by that clause alone, so deleting a clause fails loudly instead of going quietly blind.
kyleseaman
pushed a commit
that referenced
this pull request
Aug 10, 2026
The repo-wide scan parses every `.ts`/`.tsx` file under `src` -- ~1k files and ~12MB of TSX -- to find sites in a few dozen of them, and the parse is the overwhelming majority of its runtime. That cost is what made this scan too expensive to sit in the vitest suite at all: it timed out at 15650ms against the 15s per-test budget on main's Frontend Tests job (run 31359945669), which #2562 resolved by moving the scan out to a standalone gate. This removes the cost itself, so the gate that inherited it is ~2x cheaper and no longer scales with files it can never flag. A finding needs a literal chunk whose text starts with an optional space then a unit symbol, and in raw source such a chunk is always preceded by `}` (a template continuation, or JSX text after an expression) or by a quote directly after `+`. Reject a file on that raw-text test and it never gets parsed; 911 of 1026 files now don't. The unit vocabulary is read out of `UNIT` itself so the fast path cannot become narrower than the matcher, and the files where raw and cooked text can legitimately disagree -- an escape, or a comment between `+` and its operand -- are admitted unconditionally rather than reasoned about. Three smaller costs went with it: parent pointers are no longer requested from `createSourceFile` (the visitor passes down the ancestor chain it already knows), the CSS-context checks now run after the cheap unit test instead of before it, and the directory walk classifies entries from the `readdir` it already makes instead of a `stat` per entry. Measured on one box, interleaved A/B over 5 rounds, same corpus: scan phase (warm, in-process) .... 1439ms -> 434ms gate end-to-end (median) ......... 4313ms -> 2376ms The matcher is unchanged and still decides every finding. Verified equivalent against the previous implementation over all 1031 in-scope files: identical per-file hit lists, 52 findings before and after, and zero findings in any file the fast path skips. A new test pins the fast path to the matcher with a case per clause that is a real finding admitted by that clause alone, so deleting a clause fails loudly instead of going quietly blind.
iamwhatever
pushed a commit
that referenced
this pull request
Aug 10, 2026
…2592) The repo-wide scan parses every `.ts`/`.tsx` file under `src` -- ~1k files and ~12MB of TSX -- to find sites in a few dozen of them, and the parse is the overwhelming majority of its runtime. That cost is what made this scan too expensive to sit in the vitest suite at all: it timed out at 15650ms against the 15s per-test budget on main's Frontend Tests job (run 31359945669), which #2562 resolved by moving the scan out to a standalone gate. This removes the cost itself, so the gate that inherited it is ~2x cheaper and no longer scales with files it can never flag. A finding needs a literal chunk whose text starts with an optional space then a unit symbol, and in raw source such a chunk is always preceded by `}` (a template continuation, or JSX text after an expression) or by a quote directly after `+`. Reject a file on that raw-text test and it never gets parsed; 911 of 1026 files now don't. The unit vocabulary is read out of `UNIT` itself so the fast path cannot become narrower than the matcher, and the files where raw and cooked text can legitimately disagree -- an escape, or a comment between `+` and its operand -- are admitted unconditionally rather than reasoned about. Three smaller costs went with it: parent pointers are no longer requested from `createSourceFile` (the visitor passes down the ancestor chain it already knows), the CSS-context checks now run after the cheap unit test instead of before it, and the directory walk classifies entries from the `readdir` it already makes instead of a `stat` per entry. Measured on one box, interleaved A/B over 5 rounds, same corpus: scan phase (warm, in-process) .... 1439ms -> 434ms gate end-to-end (median) ......... 4313ms -> 2376ms The matcher is unchanged and still decides every finding. Verified equivalent against the previous implementation over all 1031 in-scope files: identical per-file hit lists, 52 findings before and after, and zero findings in any file the fast path skips. A new test pins the fast path to the matcher with a case per clause that is a real finding admitted by that clause alone, so deleting a clause fails loudly instead of going quietly blind. Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
encomjp
pushed a commit
to encomjp/kirocrew-customapi
that referenced
this pull request
Aug 22, 2026
kirodotdev#2562) The scan parses every in-scope file with the TypeScript compiler, so its cost scales with the size of the repo rather than with what a branch changed. Inside vitest that cost is multiplied by v8 coverage instrumentation and by every sibling worker. Measured on one 6-core box, same commit, only --maxWorkers varying: standalone gate .... 2.0 s 2 workers ......... 11.2 s 5.6x 4 workers ......... 13.7 s 6.8x <- GitHub CI shape, 15 s budget 6 workers ......... 16.6 s 8.2x 8 workers ......... 20.3 s 10.1x 12 workers ......... 28.6 s 14.2x The suite's 15 s per-test budget, sized for tests that `await import(...)`, is already marginal at the CI worker count and fails outright on a developer machine. Raising it only moves the number, because the cost tracks worker count. Throttling workers is worse: 12 -> 2 cuts the scan 60% but costs the whole suite 82% more wall clock (512 s -> 929 s). Even at 2 workers the scan pays a 5.6x tax it cannot escape, because v8 coverage is process-wide. There is no worker count at which this belongs in the suite. So it moves out, following the pattern this repo already uses for repo-scale gates (check-source-strings.mjs, check-i18n-strings.mjs, docs-lint.sh): - scripts/lib/unit-literals.mjs the matcher, shared by the gate and the tests - scripts/check-unit-literals.mjs the gate: ceiling + two diff-scoped checks - one line in the i18n gate table, which CI already runs; no workflow change - unitLiterals.test.ts keeps the four matcher tests (552 -> 75 lines, 17 ms) This is a refactor. The matcher and the diff-scoped logic are carried over unchanged, so the gate reports the same findings with the same attribution and the same limitations. Equivalence is verified rather than assumed: the old and new matchers agree file-by-file across every in-scope file, identical finding sets, nothing only-in-old and nothing only-in-new. Two behaviour notes, both called out in the issue rather than buried: The ceiling becomes `enforce: 'info'`. The gate table requires a whole-repo count that is not a hard zero to be informational, because one branch's inherited debt must not fail another branch's build. The two diff-scoped checks still stop any new site at zero tolerance. The diff-scoped markers print only when a base ref resolved. Printing them at zero with no base would claim a check that never ran, and would match the table's `find` regex and suppress its own NOT RUN branch.
encomjp
pushed a commit
to encomjp/kirocrew-customapi
that referenced
this pull request
Aug 22, 2026
…irodotdev#2592) The repo-wide scan parses every `.ts`/`.tsx` file under `src` -- ~1k files and ~12MB of TSX -- to find sites in a few dozen of them, and the parse is the overwhelming majority of its runtime. That cost is what made this scan too expensive to sit in the vitest suite at all: it timed out at 15650ms against the 15s per-test budget on main's Frontend Tests job (run 31359945669), which kirodotdev#2562 resolved by moving the scan out to a standalone gate. This removes the cost itself, so the gate that inherited it is ~2x cheaper and no longer scales with files it can never flag. A finding needs a literal chunk whose text starts with an optional space then a unit symbol, and in raw source such a chunk is always preceded by `}` (a template continuation, or JSX text after an expression) or by a quote directly after `+`. Reject a file on that raw-text test and it never gets parsed; 911 of 1026 files now don't. The unit vocabulary is read out of `UNIT` itself so the fast path cannot become narrower than the matcher, and the files where raw and cooked text can legitimately disagree -- an escape, or a comment between `+` and its operand -- are admitted unconditionally rather than reasoned about. Three smaller costs went with it: parent pointers are no longer requested from `createSourceFile` (the visitor passes down the ancestor chain it already knows), the CSS-context checks now run after the cheap unit test instead of before it, and the directory walk classifies entries from the `readdir` it already makes instead of a `stat` per entry. Measured on one box, interleaved A/B over 5 rounds, same corpus: scan phase (warm, in-process) .... 1439ms -> 434ms gate end-to-end (median) ......... 4313ms -> 2376ms The matcher is unchanged and still decides every finding. Verified equivalent against the previous implementation over all 1031 in-scope files: identical per-file hit lists, 52 findings before and after, and zero findings in any file the fast path skips. A new test pins the fast path to the matcher with a case per clause that is a real finding admitted by that clause alone, so deleting a clause fails loudly instead of going quietly blind. Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem / Motivation
src/i18n/unitLiterals.test.tsruns a whole-repo TypeScript AST scan (every non-testfile under
website/src) inside vitest's default 15s per-test budget. On a contended,many-core runner it crosses that line and fails the Frontend Tests job, as reported in
#2501 (and others). The failure is contention, not a code change: a branch that only
touches tests or locale catalogs (both excluded from the scan) can still be the one
that goes red, with nothing in the output to say so.
Why it matters
A gate that fails on timing rather than on a real violation costs a full re-run of a
~14min job (Coverage Gate consumes its artifact, so one timeout takes two checks
down), and it erodes the ratchet's authority: people learn to re-run reds instead of
reading them. It gets worse as CI and developer machines gain cores.
What changed (motivation to approach to change)
Symptom: the scan times out under contention.
Root cause: a repo-scale scan, whose cost tracks repo size and worker count and is
multiplied by process-wide v8 coverage, is living under a per-test deadline.
Raising the timeout only moves the cliff (the scan goes 2.0s standalone to 28.6s at
12 workers; full table in #2501), and throttling workers costs the whole suite +82%
wall clock. So the scan moves out of vitest into a standalone gate, which is option 3
from #2501, while the matcher's unit tests stay in vitest.
scripts/lib/unit-literals.mjs(new): the matcher, extracted verbatim so the gateand the tests share one definition and cannot drift.
scripts/check-unit-literals.mjs(new): the gate. Three checks, strongest first:[added-lines](zero tolerance),[vs-base](zero tolerance),[ceiling](info).scripts/lib/i18n-gate-table.mjs: one script row plus three check rows. No CIworkflow change; the
i18n:checkaggregator already runs the table.src/i18n/unitLiterals.test.ts: trimmed 552 to 75 lines; the four matcherassertions stay, importing the extracted module.
src/test/i18nGateTable.test.ts: fixture and assertions for the new rows.Enforcement is unchanged where it matters: the two diff-scoped checks stay zero
tolerance, so a new un-migrated literal still fails. The whole-repo ceiling is
enforce: 'info', which the gate table's contract requires for a non-hard-zerowhole-repo count, so one branch's inherited debt cannot fail an unrelated branch's build.
This is a refactor: matcher and diff logic are carried over unchanged, and equivalence is
verified rather than assumed (the gate and the tests import the same module, and the
gate's
--jsonmode emits the raw finding set for direct comparison).Tests
src/i18n/unitLiterals.test.ts: the four matcher assertions, now importingscripts/lib/unit-literals.mjs. They lock in that the matcher finds files to scan,detects the number+unit shapes that shipped, exempts CSS values, and exempts values
routed through the format seam.
src/test/i18nGateTable.test.ts: aHEALTHYfixture for the newunitsscript; aguard that throws if a registered script has no fixture; no-base
NOT RUNassertionsfor the two diff-scoped rows plus a
PASSfor the ceiling; the script-count check; andunit-ceilingin the info list.Manual verification
N/A, unit coverage is sufficient. For reference, the full suite is green (885 files,
12160 pass, 0 fail, 0 lint errors) and the gate runs standalone in 2.0s over 1026 files.
Related Issues
Fixes #2501.
Related flake context:
DevFleetPagetest in the same family of contention flakes, observed alongside this on a 16-core run)Checklist
refactor: ...)