Repository navigation
fix(ci): a pull request whose head predates the package-size check skips it visibly instead of failing build-storybook - #9191
Conversation
…ips it visibly instead of failing build-storybook The workflow comes from the merge ref and the job checks out the head, so a head cut before the check has no package-size.mjs. The measurement step now skips with a notice there, and the comparison steps run only after a measurement. size-base finds the merge base without merge-base.sh on such a head.
|
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
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The build-storybook fix works: a head without package-size.mjs now skips with a notice, and the new test fails against the old workflow and passes here. One change is needed first. The size-base fallback at .github/workflows/ci.yml:1235 runs a single git merge-base on the depth-50 fetch, so once main is 50 commits past #9161 it reports no merge base for every head it serves. Please deepen once before giving up, as merge-base.sh does, and give the size-base test a shallow clone with main more than 50 commits ahead; or drop that hunk and its line in the description.
[Automated review]
| if [ -x .github/scripts/merge-base.sh ]; then | ||
| BASE_SHA=$(.github/scripts/merge-base.sh "$BASE_REF" 'base size measurement' || true) | ||
| else | ||
| BASE_SHA=$(git merge-base "origin/$BASE_REF" HEAD || true) |
There was a problem hiding this comment.
This runs one git merge-base on the depth-50 checkout and never deepens, unlike merge-base.sh and the step before #9161. Every head reaching this branch has its merge base before #9161, so once main is 50 commits past it this always yields found=false. Deepening once restores it:
BASE_SHA=$(git merge-base "origin/$BASE_REF" HEAD 2>/dev/null) || {
git fetch --deepen=200 origin "$BASE_REF:refs/remotes/origin/$BASE_REF" >/dev/null 2>&1 || true
BASE_SHA=$(git merge-base "origin/$BASE_REF" HEAD 2>/dev/null || true)
}… size records); keep the build-storybook skip
There was a problem hiding this comment.
The build-storybook skip works for heads that predate the package-size check, and the size-base change is gone, but one change is needed before this lands. The new inline check at .github/workflows/ci.yml:1090 duplicates the test-scope action's requires-head-file, which this job already calls, and unlike it also skips when a pull request moves or deletes package-size.mjs, showing a "Head predates" notice that merging main cannot clear. Please extend requires-head-file to report its result independently of touches and gate the step on it, or say in the description why it cannot serve here and pin the path as ci-rtl-d7.test.mjs does for the RTL guard.
[Automated review]
| # from the merge ref, the checkout from the head): it skips, visibly, | ||
| # until it merges main. | ||
| run: | | ||
| if [ ! -f .github/scripts/package-size.mjs ]; then |
There was a problem hiding this comment.
This duplicates the test-scope action's requires-head-file, which this job already calls (line 945), without its rule that a pull request touching the file still runs: a PR that moves or deletes package-size.mjs would skip here with a "Head predates" notice instead of failing. Extend requires-head-file to report independently of touches and gate on it, or keep this guard, say why, and pin the path as ci-rtl-d7.test.mjs does.
…d-file (new head_file_present output) instead of an inline file check
There was a problem hiding this comment.
The fix now goes through the shared requires-head-file, which settles the earlier point, but two small corrections are needed before this lands. The head-file notice (.github/actions/test-scope/action.yml:244) now fires even when the job's work would not run, so pr-interaction-webkit, pr-interaction and stories-only build-storybook runs on older heads are told to merge main to run a check that still would not run: please word it so it holds for every caller (for example "…, so this job skips what needs it; merging main brings it in."), or emit it only when the missing file changes what the job runs. The description says every pull request cut before the check fails build-storybook, but a head whose change does not reach the package-size step, such as a Storybook-stories-only change, already passed, so please limit that claim to heads whose change reaches the step.
[Automated review]
| let headFilePresent = 'true'; | ||
| const headFile = process.env.SCOPE_HEAD_FILE; | ||
| if (applies === 'true' && headFile && !files.some(f => f.filename === headFile || f.previous_filename === headFile)) { | ||
| if (!specialized && headFile && !files.some(f => f.filename === headFile || f.previous_filename === headFile)) { |
There was a problem hiding this comment.
With the lookup no longer behind applies === 'true', the notice on line 244 now also fires when this job's work would not run: on a head cut before playwright.interaction.config.ts, pr-interaction-webkit shows "merge main to run this check" for any non-gesture component change, and pr-interaction for a README change, beside their own "No … interaction proofs for this change". Merging main would not run those proofs. Could the notice say something true for every caller, for example "This pull request's head has no ${headFile}, so this job skips what needs it; merging main brings it in."?
There was a problem hiding this comment.
The skip works and the head-file notice now holds for every job that shows it: a head without package-size.mjs skips the check with that notice, a renamed or deleted script still fails, and this head's own CI ran the package-size check. One small correction remains in the description: the Evidence bullet still says build-storybook fails on any pull request whose head predates the package-size check, but a head whose change does not reach the package-size step (a Storybook-stories-only or spec-only change) already passed. Please bound it the way the Maintainer impact paragraph does, for example "…whose head predates the package-size check and whose change reaches the package-size step."
[Automated review]
There was a problem hiding this comment.
This looks ready to merge: a head without package-size.mjs now skips the package-size check with a notice instead of failing build-storybook, a renamed or deleted script still fails, and the description now limits the failure to heads whose change reaches the package-size step. Both test files also pass when merged with current main, which changed the same scope action. One optional tidy-up: the head_file_present description could say that a specialized lane or an undecidable scope reports true without a lookup.
[Automated review]
…gates on the measurement too
There was a problem hiding this comment.
This looks ready to merge: a head without package-size.mjs now skips the package-size check with a notice instead of failing build-storybook, and a lookup error or a renamed or deleted script still runs the step and fails. After the merge with main, the new gate still covers the record download and the compare step, and this head's own build measured and compared package sizes as before. One optional tidy-up: the head_file_present description could say that a specialized lane, an undecidable scope or a failed lookup reports true without deciding.
[Automated review]
… never replaces the code-spanned package name
Maintainer impact
Since the package-size check landed, an open pull request whose head was cut before it, and whose change reaches the package-size step, fails
build-storybookwithCannot find module .github/scripts/package-size.mjs. The workflow comes from the merge ref, but the job checks out the pull request's head, which has no such script. Contributors see a red required check they did not cause. This restores them.Intended invariant
package-size.mjs, whose diff does not touch it, skips the package-size check. The scope step'srequires-head-filenotes it ("This pull request's head has no .github/scripts/package-size.mjs, so this job skips what needs it; merging main brings it in."), the same convention the interaction lanes use. The merge-base and comparison steps after it run only once the head was measured.Change
test-scopeaction: a newhead_file_presentoutput reports whether the head hasrequires-head-file, whether or nottouchesholds.appliesstill folds it in.ci.ymlbuild-storybook: the scope step names.github/scripts/package-size.mjsas its required head file, and the measure step gates onhead_file_present. The measure step setsmeasured=trueonly after measuring, and the two later steps gate on that output instead of the step's outcome.Evidence
build-storybookfails on a pull request whose head predates the package-size check and whose change reaches the package-size step.test-scope-action.test.mjsshowshead_file_presentis false for a head without the file, whether or nottouchesholds, and true for a head that has it or a diff that renames or deletes it. The new contract test pins the scope input and the measure gate, and runs the measure step's own script (measurement recorded).Scope
Testing
vitest run --project node .github/scripts: all pass except one Chromium-launch test this machine cannot run (rtl-audit-cli, unrelated).ci-package-size-predating-head.test.mjs(3 tests), and 2 new tests intest-scope-action.test.mjs.actionlint: no new findings.