Repository navigation
ci: a pull request that changes CI shows its CI impact: CI workflow job-minutes, runner-pool usage and time to green, and runner classes of what changed (AST-030 FR19) - #9187
Conversation
…h PR, in the build job Measures every published package's npm download and unpacked size in build-storybook, after its build, and compares it with the merge base's measurement that Main now keeps for 14 days. Adds a Published Package Size section to the PR report and the job summary, and fails the job when a package's download grows more than 5% (and at least 1 KB) unless the PR description has a 'Size budget:' line.
…e ci:heavy-lanes Each label opts a pull request into one heavy suite. A run-labels gate job reads the event's labels and, for each suite label, the newest labeler's role (write, maintain or admin; fail closed), from a decision script in the merge commit's sparse checkout; each suite's owners need it and run only on their own suite's output. A label still takes effect on the next push or reopen, ci:run stays the fork gate, and the release check is unchanged. The pull-request comment names the label for a suite that did not run. AST-030, CONTRIBUTING and the visual and RTL docs name the new labels.
…ize, visual, accessibility, RTL, perf when it ran, and the CI jobs; touched components link their docs and stories
…helper; the check also follows root build inputs - A package over its budget is flagged (job summary, PR report, a workflow warning) and fails nothing, like the component touch-only flags; the 'Size budget:' line and the enforce step are gone. - The package-size merge-base step deepens once when the 50-commit fetch holds no merge base, as size-base does. - Report counts render through num(). - tsconfig.json and scripts/ are in the check's paths scope. - size-base and the package-size lookup share lib/main-artifact.cjs.
…ip reason is its own skip step
…ied-components table (name links docs, Storybook, size, LoC, Visual · A11y · RTL marks), collapsed visual thumbnails, package size lines, one not-run hint Changed shots' frames are published to the PR's assets branch by publish-frames.mjs so the report can show before/after/diff thumbnails; the touched-components report (wide-change flag included) is read from its own artifact.
…s/pr-<n>, and the daily pruner removes that branch once its PR closes
…rked .pr-report-frames); hand-pushed assets branches keep the existing rule
…lass per changed job, job-minutes per update, runner-pool usage, time to green and runs a day, before and after (AST-030 FR18) The comment job measures it from the workflow change and the last 20 successful pull-request runs, kept as a daily artifact so later CI-changing pull requests that day reuse it. It reports runner usage, never prices. The maintenance template and CONTRIBUTING ask for a one-line 'CI impact:'.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
cixzhang
changed the title
|
/approve-spec 2686b26 |
github-actions
Bot
removed
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface SearchContext |
interface SearchContext<T extends SearchableItem = SearchableItem> |
import type {SearchContext} from '@astryxdesign/core' |
| + | core |
./Typeahead · Also exported from an earlier entry point · SearchContext |
SearchContext (.) |
import {SearchContext} from '@astryxdesign/core/Typeahead' |
| ~ | core |
. · Types · interface SearchSource |
search: (query: string) => Promise<T[]> | T[] → search: (query: string, context?: SearchContext<T>) => Promise<T[]> | T[] |
import type {SearchSource} from '@astryxdesign/core'; const x: SearchSource = {search: …} |
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
…r runners) and compares time to green only for a successful run
|
/approve-spec 967ec74 |
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: FR19 is owner-approved and the earlier runner-label, summary-line and title fixes hold, but three things need fixing before this can land.
- Merge
mainand start the new "Find today's CI history" script with the shared rate-limit wait (installRateLimitBackoff({github, core})), because without it the requiredtestcheck fails inrate-limit-backoff.test.mjs("pr-comment.yml comment installs the wait before its first request"). - Look the history artifact up through
mainArtifactRunIdin.github/scripts/lib/main-artifact.cjs, with itsshamade optional, instead of copying its trust filter into the workflow. - When same-named jobs merge, report a variant that always runs on a larger runner as non-standard rather than "standard or non-standard by event", with a test on the rendered line.
[Automated review]
| const name = `ci-history-${new Date().toISOString().slice(0, 10)}`; | ||
| const {data} = await github.rest.actions.listArtifactsForRepo({...context.repo, name, per_page: 20}); |
There was a problem hiding this comment.
Once main is merged, rate-limit-backoff.test.mjs requires every github-script step in this job to start with the shared wait, and this one makes an API call without it, so the required test check fails. Start the script with:
// Wait out the repository's shared API rate limit (lib/rate-limit-backoff.cjs).
require(require('node:path').join(process.env.GITHUB_WORKSPACE, '.github/scripts/lib/rate-limit-backoff.cjs'))
.installRateLimitBackoff({github, core});| // Only this repository's own runs on main keep the history. | ||
| const artifact = data.artifacts.find(candidate => | ||
| !candidate.expired && | ||
| candidate.workflow_run?.head_branch === 'main' && | ||
| candidate.workflow_run?.head_repository_id === candidate.workflow_run?.repository_id); |
There was a problem hiding this comment.
This is the trust filter mainArtifactRunId in .github/scripts/lib/main-artifact.cjs already owns (unexpired, head_branch === 'main', same repository). Making its sha optional and calling it here keeps one filter for every lookup of an artifact Main keeps, so a later trust fix cannot miss this one.
| function mergeClasses(a, b) { | ||
| if (!a) return b; | ||
| if (!b) return a; | ||
| return classifyLabels([...a.labels, ...b.labels]); | ||
| } |
There was a problem hiding this comment.
Reclassifying the union of labels turns a variant on ubuntu-latest beside one on 4-core-ubuntu into mixed, so the per-id fixture in ci-impact.test.mjs renders pr-a11y changed: standard or non-standard by event (4-core-ubuntu) and the summary says "some runners chosen by event or unknown". Merging by rank (a wholly non-standard variant makes the entry non-standard) and asserting the rendered line would fix it.
…runs through main-artifact.cjs (commit now optional); a variant always on a larger runner makes its job non-standard
|
/approve-spec 9dba1d1 |
There was a problem hiding this comment.
Changes requested: FR19 is owner-approved and the rate-limit, shared-lookup and merged-runner fixes are in, but the CI impact section still misreports two cases.
- A pull request that changes only a composite action or a CI script, with
ci.ymlunchanged, reads0 job-min per update · pool 0%and never trips the pool flag; count jobs that use a changed local action as changed, or print "not measured" for those changes, and add a fixture for it. - Runs a day counts 7 to 8 days of runs and divides by 7, so runner-minutes a day and concurrent jobs read up to 14% high; query the seven whole days before today.
[Automated review]
| */ | ||
| function estimateImpact({diff, history, currentJobs, currentWallMinutes}) { | ||
| const current = runMinutes(currentJobs); | ||
| const changed = new Set( |
There was a problem hiding this comment.
changed holds only jobs whose ci.yml definition changed, so a pull request that changes only a composite action (such as ./.github/actions/setup, which most ci.yml jobs call) or a CI script keeps every job at its history average and reads 0 job-min per update · pool 0% with no pool flag, even when this run took several minutes more per job. Counting a job as changed when a step uses: a changed local action, and printing "not measured" for changes the report cannot attribute (as the other-workflow note already does), would keep FR19's pool flag honest.
| now = new Date(), | ||
| }) { | ||
| if (cached?.runs) return cached; | ||
| const since = new Date(now.getTime() - 7 * 24 * 3600 * 1000) |
There was a problem hiding this comment.
Truncating to a date makes created: >=<date> cover 7 to 8 days depending on the time of day, and summarizeHistory divides by 7, so runs a day, runner-minutes a day and concurrent jobs read up to 14% high late in a UTC day. Querying the seven whole days before today (created: <today-7>..<today-1>) matches the Assumptions line.
…sured, and counts runs a day over the seven whole days before today
|
/approve-spec 07adc33 |
There was a problem hiding this comment.
Changes requested: FR19 is owner-approved and the earlier not-measured and runs-a-day fixes are in, but the CI impact section still misreports two cases.
- A changed job's minutes and time to green come from this one run, so an edit to a label-gated suite tested with its label reads
pool +16% · time to green +10 minand trips both flags although later pull requests run nothing new; weight changed jobs by how often they ran in the sampled runs, compare time to green only with runs that ran the same jobs, and add a fixture for it. - A pull request that changes only
crowdin-upload.ymlis routed as tooling-only and gets no CI impact section; post the section from that path, or scope FR19 and the description to leave FR8's tooling admissions out.
[Automated review]
| const before = sum(diff.names.before, name => history.jobMinutes[name] ?? 0); | ||
| const after = sum(diff.names.after, name => | ||
| changed.has(name) | ||
| ? (current[name] ?? history.jobMinutes[name] ?? 0) |
There was a problem hiding this comment.
This run's minutes stand in for every later pull request, but a label-gated suite the author opts into (ci:run-visual and friends) runs here while almost every sampled run skips it, so its full minutes are compared with an average that counts those skips as zero. On the real ci.yml, an env-only edit to pr-visual tested under its label reads +13.3 job-min per update · pool +16% · time to green +10 min ⚠️. Weighting a changed job's minutes by the share of sampled runs in which it ran (when its if: is unchanged), and comparing time to green only with runs that ran the same jobs, would keep both flags to real added load.
…time to green with runs of the same jobs; FR19 states the node-tooling lane gets no comment
|
/approve-spec 50559d3 |
github-actions
Bot
added
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: FR19 is owner-approved, but the new top-level yaml import in .github/scripts/lib/pr-preview.mjs cannot load in the PR Comment workflow's resolve and spec-only-reconcile jobs, which run without a dependency install, so every pull request would lose its report and get a failing PR Comment status. Loading the parser where it is used (inside workflowAt) and adding a test that pr-preview.mjs imports only Node built-ins and relative files would fix it.
- A changed job that ran in none of the 20 sampled runs is counted as if every later pull request ran it (
shareinci-impact.js), which FR19's "counts as often as it ran in those runs" rules out. - FR19 and
CONTRIBUTING.mdsay the comment measures any workflow change, but onlyci.ymlis measured; naming other workflows as not measured in both would match the code.
A new commit will need a fresh /approve-spec for its head.
[Automated review]
| import process from 'node:process'; | ||
| import {createRequire} from 'node:module'; | ||
| import {fileURLToPath} from 'node:url'; | ||
| import yaml from 'yaml'; |
There was a problem hiding this comment.
The PR Comment workflow's resolve and spec-only-reconcile jobs import this module from a main checkout with no setup step, so there is no node_modules; loaded that way this line throws ERR_MODULE_NOT_FOUND Cannot find package 'yaml' (the base module loads). Every pull request would then lose its report and get a failing PR Comment status. Loading it inside workflowAt (const {parse} = await import('yaml')), which only the installed comment job reaches, and adding a test that this module imports only node: built-ins and relative files would fix it.
| const share = name => | ||
| history.sampled > 0 && history.jobMinutes[name] !== undefined | ||
| ? (history.jobRuns?.[name] ?? 0) / history.sampled | ||
| : 1; |
There was a problem hiding this comment.
A changed job absent from the sampled runs gets a share of 1, so a label-gated suite that ran in none of the 20 runs, edited and tested under its label, reads as fully added (+12 job-min per update · pool +27% ⚠️ in a probe with pr-rtl skipped in every sampled run). FR19 says a changed job "counts as often as it ran in those runs", which is zero here. A share of 1 only for added jobs, 0 for a changed job with no sampled runs, and a fixture for it would match.
| pull-request comment measures these from the workflow change and recent | ||
| pull-request runs, without starting a job: a changed job counts as often as | ||
| it ran in those runs, time to green compares with runs that ran the same | ||
| jobs, and a changed composite action or CI script is named as not measured. |
There was a problem hiding this comment.
Only ci.yml is measured; gatherCiImpact names every other changed workflow "not measured", including pull-request workflows such as lint.yml. Adding "or workflow other than ci.yml" to the not-measured clause here and in CONTRIBUTING.md keeps the record to what the comment does.
…ports only built-ins and relative files, pinned), a changed job that never ran counts zero, and FR19 and CONTRIBUTING say only ci.yml is measured
|
/approve-spec 7950582 |
There was a problem hiding this comment.
Changes requested: FR19 is owner-approved and the earlier fixes hold, but two things need fixing before reviewers can rely on the CI impact section.
- Renaming a
ci.ymljob makes its minutes read as removed (a job that grows from 10 to 14 minutes shows−10 job-min per update · pool −83%with no flag), because its share of the sampled runs is looked up under the new name; take it under the base-side name and add a rename fixture. pr-preview-imports.test.mjsonly seesrequire(on the same line asconst, so a multi-line require like the one inci-impact.jsslips past it; importpr-preview.mjswhere nonode_modulesresolves, or match requires across lines.
A new commit will need a fresh /approve-spec for its head.
[Automated review]
| const share = name => | ||
| added.has(name) || history.sampled === 0 | ||
| ? 1 | ||
| : (history.jobRuns?.[name] ?? 0) / history.sampled; |
There was a problem hiding this comment.
A renamed job (same id, new name:) is looked up here under its head-side name, which no sampled run has, so it counts zero while before keeps its minutes under the old name. A job that grew from 10 to 14 minutes reads −10 job-min per update · pool −83% with no flag, though FR19 says a changed job counts as often as it ran. Carrying the base-side name on each change in diffWorkflow, taking the share under it, and a rename fixture would fix it.
| // Top-level requires only (not inside a function body): column 0 or a | ||
| // top-level const. | ||
| for (const match of source.matchAll( | ||
| /^(?:const|let|var)[^\n]*?\brequire\(\s*['"]([^'"]+)['"]\s*\)/gm, |
There was a problem hiding this comment.
This needs require( on the same line as const, so the multi-line destructured require at ci-impact.js:21-25 is never seen and runner-labels.cjs is never visited: prepending a multi-line require('yaml') to ci-impact.js keeps this test green, though the uninstalled resolve job would then fail to load the module. Importing pr-preview.mjs from a copy of .github/scripts where no node_modules resolves would test the real property.
…de name, and the import contract sees requires across lines
|
/approve-spec bb1fbb6 |
There was a problem hiding this comment.
Approval recommended: FR19 is owner-approved at this head, and both earlier findings are fixed, each pinned by a test that fails without the fix. A renamed ci.yml job now keeps its history under its base-side name, and the import contract test catches multi-line requires. A new commit will need a fresh /approve-spec for its head.
[Automated review]
…tails, the CI impact section stays outside the budget, and the placeholder shows as before
Maintainer impact
A pull request that changes CI can make every later pull request slower or put more load on the shared runner pool. Today nothing on the pull request shows that, so reviewers judge workflow changes blind. This adds a CI impact section to the PR report for CI changes, measured from data the comment job can already read, and asks such pull requests for a one-line
CI impact:in their description (AST-030 FR19).CI impact: standard runners only. No CI job is added or changed. The PR Comment job gains three short artifact steps (seconds per report). Job-minutes per PR update, runner-pool usage and time to green are unchanged.
Intended invariant
lib/runner-labels.cjs). Jobs that share a name share one entry with their classes merged.node-toolingpull request (AST-030 FR8) gets no PR comment, so FR19 asks only for its description line.ci.ymlis measured. Other changed workflows, composite actions and CI scripts are named as not measured, both in the section and in FR19 and CONTRIBUTING.pr-preview.mjsloads the YAML parser lazily, because the PR Comment workflow's resolve and spec-only jobs import it without installing dependencies. A contract test (pr-preview-imports.test.mjs) pins that it, and everything it loads at the top level, imports only Node built-ins and relative files.Change
lib/ci-impact.js(new) covers:runs-onexpression can yield);ci.ymlat the merge base and the head;lib/pr-preview.mjs:gatherCiImpactreads the changed files,ci.ymlat the merge base and head, and the source run.ciHistorysamples the last 20 successful pull-request CI runs' jobs and counts 7 days of runs that did work.pr-comment.yml: the comment job keeps the history as aci-history-<date>artifact, so later CI-changing pull requests that day reuse it. It looks the artifact up, downloads it, and keeps it when it computed a new one. There is no new job. Every report adds one file-list read. A CI change adds the merge-base, workflow-file and run reads, plus about 23 history reads once a day.CI impactevidence line, CONTRIBUTING describes it, and AST-030 gains FR19 with its verification row.Evidence
run-labelsreads "standard or non-standard by event" (the release-check pool), so the summary reads "some runners chosen by event or unknown".Scope
Writing rubric results
Run over
docs/specs/AST-030/spec.md: FR19 and its verification row.ci-impact.test.mjs,pr-preview.test.mjs), not this pull request.Testing
vitest run --project node .github/scripts: all pass except one Chromium-launch test this machine cannot run (rtl-audit-cli, unrelated).scripts/check-knowledge.mjspasses.lib/ci-impact.test.mjscovers:pr-preview.test.mjs: a CI change reaches the comment with the merge base as before; a non-CI change passes no impact.