Repository navigation
ci: required lint takes its spec and docsite fast paths - #8880
Conversation
lint.yml classifies a pull request's files with the trusted base classifier and skips full ESLint for a spec-only, docsite-only or tooling-only change. Its depth-1 checkout never let the merge commit reach its merge base, so the classifier was never fed and every pull request ran full lint (about four minutes of ESLint on a spec-only change). The checkout now takes the same 50-commit history ci.yml's check-scope does. A contract test runs the real step and classifier with real git.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The 50-commit checkout makes required lint's spec-only and docsite-only fast paths fire as spec:AST-030 FR3 intends, and the new contract test proves it with the real step and classifier; one wording fix is needed before this lands. The new test's header (.github/scripts/ci-lint-scope.test.mjs, lines 4-9) and the PR title say required lint skips full ESLint for tooling-only changes, but no lint step reads tooling_only and FR8 requires tooling-only changes to run the full lint workflow, so please describe only the spec and docsite fast paths there. Also worth knowing before merge: docsite-only pull requests will now skip ESLint in pull-request CI entirely, including the docsite iframe-sandbox and target-blank rules, as FR2's docsite owner list allows.
[Automated review]
| * @file Contract: required lint takes its documentation, spec and tooling fast | ||
| * paths. | ||
| * | ||
| * lint.yml classifies a pull request's files with the trusted classifier from | ||
| * the base branch and skips full ESLint for a spec-only, docsite-only or | ||
| * tooling-only change. Its checkout was depth 1, where the merge commit never |
There was a problem hiding this comment.
Required lint has no tooling fast path: no lint.yml step reads tooling_only, so a tooling-only change runs setup, guardrails, the CLI README check and ESLint, as spec:AST-030 FR8 requires and ci-tooling-routing.test.mjs pins. Suggest naming only the spec and docsite fast paths here (and in the PR title):
* @file Contract: required lint takes its documentation and spec fast paths.
*
* lint.yml classifies a pull request's files with the trusted classifier from
* the base branch and skips full ESLint for a spec-only or docsite-only change;
* a tooling-only change still runs it (spec:AST-030 FR8). Its checkout was
* depth 1, where the merge commit never| ).toMatchObject({ | ||
| spec_only: 'false', | ||
| docsite_only: 'false', | ||
| tooling_only: 'false', | ||
| }); |
There was a problem hiding this comment.
Non-blocking: this case also passes against the base workflow, because the no-merge-base fallback writes the same three falses. Adding full_suite: 'false', which only the classifier writes, would show the classifier answered at the workflow's depth:
).toMatchObject({
spec_only: 'false',
docsite_only: 'false',
tooling_only: 'false',
full_suite: 'false',
});… classifier answered
cixzhang
changed the title
There was a problem hiding this comment.
The 50-commit checkout makes required lint's spec-only and docsite-only fast paths fire as spec:AST-030 FR3 intends, and the updated contract test now fails against the old workflow and passes with the change; the header and title wording raised last time is fixed, so this looks good to merge once the required checks report. No GitHub Actions run has appeared for the latest commit yet, so lint, test and the other required checks still need to run. Worth knowing before merging: docsite-only pull requests will now skip ESLint (including the docsite iframe-sandbox and target-blank rules) and the repository guardrails in pull-request CI, as FR2's docsite owners allow.
[Automated review]
Maintainer impact
A spec-only or docsite-only pull request should skip full ESLint in required
lint(a tooling-only one still runs it, per AST-030 FR8); lint.yml already has those fast paths. They never fired. Its checkout was depth 1, and on a depth-1 PR merge commitgit merge-base HEAD origin/mainfinds nothing. So the step fell back to "run full lint" on every pull request. Specs-only PR #8632 spent 197 s in ESLint, plus a 97 s checkout, for one changedspec.md. Now those PRs get the lint they were meant to get, and the requiredlintcontext still reports from real work on every PR.Intended invariant
lint.yml feeds the trusted base classifier the same diff ci.yml's
check-scopedoes: the merge commit against its merge base with the base branch. A spec-only or docsite-only change takes its existing fast path; a tooling-only change keeps full lint (FR8). Anything else, or a classifier that can't run, still runs full lint (fail-safe unchanged).Change
fetch-depth: 50, the history ci.yml'scheck-scopechecks out, so the merge commit reaches its merge base. Nothing else changes.ci-lint-scope.test.mjsbuilds a real repository in which main moves on after the PR branches, checks out the way lint.yml does, and runs the real step and classifier. A spec-only PR givesspec_only=true, and a PR that also changes a component gives full lint.Evidence
refs/pull/8632/merge, after fetching main at depth 50, finds no merge base..github/scriptsNode suite passes. Two Chromium-timing tests inrtl-audit-cliflaked under machine load and pass when run alone.Scope
Testing
pnpm vitest run --project node .github/scripts/ci-lint-scope.test.mjs .github/scripts/ci-tooling-routing.test.mjsLines of code
CI files (
.github/**): +170 / −0, net +170. Justification: a one-line checkout fix; the added lines are a real-git contract test proving the classifier now runs.