Repository navigation
fix(ci): head-checkout jobs guard each script a stale head may lack - #9288
Conversation
Jobs that check out the pull request's head run the repository scripts at that head. A branch cut before a script existed failed with "Cannot find module". Each newer script a head-checkout job runs is now guarded with hashFiles, beside a notice step that says merging main runs it: the API change report, the browser accessibility specs, the D7 native target check and the RTL scorecard summaries; the package-size comparison reads the scope's head_file_present. A contract test fails when a head-checkout job runs a script that is neither established nor guarded.
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. The guards on the API change report and the two RTL scorecard summaries fix real stale-head failures, but two of the five guarded steps already handled a head without the script, and four existing ci.yml contract tests now fail (test-node (2/4) and test-node (4/4)).
- Drop the guard on Run browser accessibility specs: the step loads its runner from the merge commit, so the guard skips a labelled pull request's specs on older heads.
- Revert the D7 guard: that step already skips a head without its test, and
ci-rtl-d7.test.mjspins its condition. - Give each notice step its guarded step's conditions, because the
cli.mjsnotice currently prints on every docsite-, spec- or tooling-only pull request. - Narrow the description to what the contract test covers:
.js/.mjs/.cjsscripts named inrun:steps.
[Automated review]
|
|
||
| - name: Run browser accessibility specs | ||
| if: (!cancelled()) && steps.scope.outputs.has_components == 'true' | ||
| if: ((!cancelled()) && steps.scope.outputs.has_components == 'true') && hashFiles('.github/scripts/a11y-pr-contracts.mjs') != '' |
There was a problem hiding this comment.
This step never runs the head's copy of a11y-pr-contracts.mjs: it extracts the runner from $WORKFLOW_SHA below, and the runner's header says it serves heads that lack it. With this guard, a ci:run-a11y pull request on a branch cut before the runner landed runs no specs and pr-a11y still passes. It also breaks ci-test-routing.test.mjs:369 and release-check-workflow.test.mjs:97. Suggest dropping the guard and the notice above.
| # A head cut before cli.mjs existed cannot run the next step; | ||
| # say so (merging main runs it). | ||
| - name: Note a head that predates cli.mjs | ||
| if: hashFiles('scripts/api-report/cli.mjs') == '' |
There was a problem hiding this comment.
In docsite-, spec- and tooling-only lanes the head checkout is skipped, so this file is never in the workspace and the notice prints on every such pull request. Give it the same specialized-lane and pull_request conditions as the step it describes; that also fixes ci-tooling-routing.test.mjs:185.
|
|
||
| - name: Verify D7 native target regression | ||
| if: (matrix.package == 'core' && matrix.part == 1) && (steps.scope.outputs.has_rtl_components == 'true' || steps.scope.outputs.has_rtl_harness == 'true' || steps.scope.outputs.force_full_component_audits == 'true') | ||
| if: ((matrix.package == 'core' && matrix.part == 1) && (steps.scope.outputs.has_rtl_components == 'true' || steps.scope.outputs.has_rtl_harness == 'true' || steps.scope.outputs.force_full_component_audits == 'true')) && hashFiles('.github/scripts/rtl-audit-coarse-hit.browser.test.mjs') != '' |
There was a problem hiding this comment.
The step's script already skips a head without the D7 test and says why (the [ ! -f "$D7_TEST" ] branch below), and ci-rtl-d7.test.mjs:72 pins this exact condition. This guard makes that branch unreachable, and the new notice prints on all seven legs. Suggest reverting this step.
| if ( | ||
| script === requiredHeadFile && | ||
| condition.includes('head_file_present') | ||
| ) { |
There was a problem hiding this comment.
Only run: text is scanned, so a script read from the merge commit (git show "$WORKFLOW_SHA:…") or guarded inside its own script counts as unguarded. Meanwhile .sh scripts and github-script require calls (merge-base.sh, lib/main-artifact.cjs) aren't seen at all.
…eir step The browser accessibility specs load their runner from the workflow's own commit, and the D7 check skips a head without its test on its own, so both guards are dropped and those steps match main. Each remaining notice step carries its guarded step's conditions, so it prints only where the step would have run. The contract test accepts a script loaded from WORKFLOW_SHA or a step that skips a missing script itself, and says it covers .js, .mjs and .cjs scripts named in run steps.
|
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 ·
|
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface FieldStatusProps |
message: string |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {message: …} |
| + | core |
. · Types · interface FieldStatusProps |
ref?: React.Ref<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {ref: …} |
| + | core |
. · Types · interface FieldStatusProps |
type: InputStatusType |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {type: …} |
| + | core |
. · Types · interface FieldStatusProps |
variant?: FieldStatusVariant = 'attached' |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {variant: …} |
| + | core |
. · Types · interface FieldStatusProps |
xstyle?: StyleXStyles |
import type {FieldStatusProps} from '@astryxdesign/core'; const x: FieldStatusProps = {xstyle: …} |
| + | core |
. · Types · interface FieldStatusProps |
inherits AriaAttributes, DOMAttributes, HTMLAttributes |
import type {FieldStatusProps} from '@astryxdesign/core' |
| + | 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 |
. · Components · component ComplexSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<ComplexSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component DateInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateInput statusVariant="attached" /> |
| ~ | core |
. · Components · component DateRangeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<DateRangeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Field |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Field statusVariant="attached" /> |
| ~ | core |
. · Components · component FieldStatus |
component FieldStatus(props: FieldStatusProps) → component FieldStatus @deprecated |
<FieldStatus /> |
| ~ | core |
. · Components · component FieldStatus |
prop variant?: FieldStatusVariant = 'attached' → prop variant: FieldStatusVariant = 'attached' |
<FieldStatus variant="attached" /> |
| ~ | core |
. · Components · component FileInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<FileInput statusVariant="attached" /> |
| ~ | core |
. · Components · component MultiSelector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary and ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary and ghost selectors |
<MultiSelector statusVariant="attached" /> |
| ~ | core |
. · Components · component NumberInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<NumberInput statusVariant="attached" /> |
| ~ | core |
. · Components · component PowerSearch |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<PowerSearch statusVariant="attached" /> |
| ~ | core |
. · Components · component Selector |
prop statusVariant?: FieldStatusVariant = 'attached' for input selectors; 'detached' for secondary/ghost selectors → prop statusVariant?: FieldStatusPlacement = 'attached' for input selectors; 'detached' for secondary/ghost selectors |
<Selector statusVariant="attached" /> |
| ~ | core |
. · Components · component TextArea |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextArea statusVariant="attached" /> |
| ~ | core |
. · Components · component TextInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TextInput statusVariant="attached" /> |
| ~ | core |
. · Components · component TimeInput |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<TimeInput statusVariant="attached" /> |
| ~ | core |
. · Components · component Tokenizer |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Tokenizer statusVariant="attached" /> |
| ~ | core |
. · Components · component Typeahead |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusPlacement = 'attached' |
<Typeahead statusVariant="attached" /> |
| ~ | core |
. · Types · interface FieldStatusProps |
interface FieldStatusProps extends BaseProps<HTMLDivElement> (props of FieldStatus) → interface FieldStatusProps extends BaseProps<HTMLDivElement> |
import type {FieldStatusProps} from '@astryxdesign/core' |
| ~ | core |
. · Types · interface FieldStatusVariantMap |
tooltip: true → tooltip: true @deprecated |
import type {FieldStatusVariantMap} from '@astryxdesign/core'; const x: FieldStatusVariantMap = {tooltip: …} |
| ~ | 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: …} |
| ~ | core |
. · Types · interface UseInputStatusIconOptions |
statusVariant?: FieldStatusVariant → statusVariant?: FieldStatusPlacement |
import type {UseInputStatusIconOptions} from '@astryxdesign/core'; const x: UseInputStatusIconOptions = {statusVariant: …} |
| ~ | richtext |
. · Components · component RichTextEditor |
prop statusVariant?: FieldStatusVariant = 'attached' → prop statusVariant?: FieldStatusVariant | 'tooltip' = 'attached' |
<RichTextEditor statusVariant="tooltip" /> |
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
CI impact: +0.6 job-min per update · pool +0.8% · some runners chosen by event or unknown · 1 CI action or script file changed (not measured)
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 78.7 | 79.3 | +0.6 |
| Runner pool, runner-minutes a day | 36,690 | 36,988 | +297 (+0.8%) |
| Average concurrent jobs | 25.5 | 25.7 | +0.2 |
| Time to green (median → this run) | — | 15.8 min | — |
| Runs a day | 466 | 466 | 0 |
Changed jobs: build-storybook changed: standard or non-standard by event (4-core-ubuntu) · pr-rtl changed: standard or non-standard by event (2-core-ubuntu-arm, 4-core-ubuntu)
Not measured (the jobs that use these keep their recent minutes above): .github/scripts/ci-head-checkout-scripts.test.mjs.
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
Requesting changes to the description only. The guards and the new contract test hold up: the accessibility and D7 steps are back to their base behaviour, and every ci.yml contract test passes. But the User impact says this closes red stale-branch jobs, and none of the three guarded steps made a job red: Report API changes is continue-on-error, and a head without rtl-scorecard-summary.mjs already fails build-storybook on build-packages-and-storybook.mjs, so neither scorecard step runs there. Please reword User impact to say that the API report now skips with a notice and that the lasting change is the contract test for newly added scripts.
[Automated review]
|
|
||
| - name: Write scorecard summary | ||
| if: (always() && steps.rtl-scope.outputs.should_run == 'true' && matrix.parts == 1) && (steps.scope.outputs.has_rtl_components == 'true' || steps.scope.outputs.has_rtl_harness == 'true' || steps.scope.outputs.force_full_component_audits == 'true') | ||
| if: ((always() && steps.rtl-scope.outputs.should_run == 'true' && matrix.parts == 1) && (steps.scope.outputs.has_rtl_components == 'true' || steps.scope.outputs.has_rtl_harness == 'true' || steps.scope.outputs.force_full_component_audits == 'true')) && hashFiles('.github/scripts/rtl-scorecard-summary.mjs') != '' |
There was a problem hiding this comment.
This guard never changes an outcome: a head without rtl-scorecard-summary.mjs (#7419) also predates build-packages-and-storybook.mjs (#7447), so build-storybook fails first and this job skips. It keeps the contract test green, which is fine, but the description shouldn't count it as a fix. Listing the script as established would also work, since it's older than build-packages-and-storybook.mjs, and that would drop both scorecard guards.
| /\[ ! -f "?\$?[A-Za-z0-9_]*"? \]/.test(run) && | ||
| run.includes('exit 0') |
There was a problem hiding this comment.
Non-blocking: this exempts every script in the step once any [ ! -f "$X" ] and any exit 0 appear, even when the check names a different file. Tying it to the variable that holds the script would keep a later step from slipping through.
There was a problem hiding this comment.
Recommending approval. The reworded User impact now matches what the guards do: the API change report swaps a non-fatal error line for a notice, and the contract test is the lasting change. All ci.yml contract tests pass and the required checks are green. One optional follow-up: the test's in-step skip exemption (any [ ! -f ... ] plus exit 0) currently exempts every script in that step, so tying it to the variable that holds the script would keep a later step from slipping through.
[Automated review]
…hecks The exemption matched any `[ ! -f ... ]` plus `exit 0` and so covered every script in the step. It now requires the check to name this script, directly or through the variable that holds it, and a case proves a second script in the same step is still caught.
There was a problem hiding this comment.
Recommending approval. The in-step skip exemption is now tied to the script its check names, which closes the last open point, and the contract test passes on this head and when merged with current main. One optional follow-up: the description's authority line attributes "a skipped check says why" to AST-030 FR12 and FR13, which don't say that, so noting that no AST-030 clause covers stale heads would be more accurate.
[Automated review]
User impact
scripts/api-report/cli.mjs. That step iscontinue-on-error, so this replaces a confusing error line, not a red job. The RTL scorecard guards cover the same case for heads that build. A head old enough to lackrtl-scorecard-summary.mjsalready fails earlier, inbuild-storybook.Problem and solution fit
hashFiles('<script>') != ''.head_file_present.git show "$WORKFLOW_SHA:…").ci-rtl-d7.test.mjs)..js,.mjsand.cjsscripts named in a step'srun:. It fails when a head-checkout pull-request job runs one that is neither established nor guarded. Guarded means ahashFilescondition, the scope'shead_file_present, loading fromWORKFLOW_SHA, or the step's own missing-file skip that names that script (directly or through its variable).Expected behavior and authority
spec:AST-030FR12 and FR13: a pull request runs what its change reaches, and a skipped check says why. A check never reads as failed because the branch predates it.Change
.github/workflows/ci.yml, guarded withhashFilesplus a notice step that carries the step's own conditions:scripts/api-report/cli.mjs)..github/scripts/rtl-scorecard-summary.mjs)..github/scripts/rtl-scorecard-summary.mjs).head_file_present, as Measure does..github/scripts/ci-head-checkout-scripts.test.mjs(new):pull_request.head.sha(release-check-only jobs and the release bump excluded).hashFilesguard to have a notice step with the guarded step's conditions.CI impact: no runner, job, runner-pool or time-to-green change; the guarded steps run exactly as before on any head that has their script. A guard adds one notice step that runs in milliseconds.
Evidence
ci-rtl-d7.test.mjspasses.pnpm vitest run --project node .github/scripts/: all pass except one visual-gate timing case that passes on its own under load.Scope