Fix link check on pushes that have no base commit - #21644
Merged
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21644
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New Failures, 9 Unclassified FailuresAs of commit 71d139a with merge base 812c7f0 ( NEW FAILURES - The following jobs have failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
shoumikhin
force-pushed
the
link-check-base-ref
branch
from
August 7, 2026 14:32
721b4f2 to
7a1237a
Compare
_link_check.yml inferred which commits to diff from the event that triggered its caller. A push creating a tag or a branch reports an all zero base SHA, so the fetch failed with exit 128 and no linting ran. A scheduled or manual run has no base at all, and reached a whole tree scan only because an unquoted command substitution happened to drop the empty argument. Pass the base in explicitly. Empty means whole tree, which the lint scripts already support, and the all zero SHA maps to empty. The fetch folds into the lint step, so each job is one step shorter than before. nightly.yml runs the check only on the schedule and on a manual dispatch. Its pull request runs were checking nothing: it passes the merge commit as the ref, so the head commit is never fetched and git diff base..head cannot resolve. lint_urls.sh and lint_xrefs.sh swallowed that and passed having read zero URLs, while lint_file_size.sh failed hard on every such run. Pull requests get the real check from lint.yml, against their own base.
shoumikhin
force-pushed
the
link-check-base-ref
branch
from
August 7, 2026 15:05
7a1237a to
71d139a
Compare
Gasoonjia
approved these changes
Aug 7, 2026
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
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
.github/workflows/_link_check.ymldecides which commits to diff by inspecting the event that triggered its caller. That is the wrong place to make the decision. A reusable workflow is called from more than one context, and the value it reaches for is not present in all of them.For a pull request it uses the pull request base. For everything else it uses
github.event.before, which is only meaningful for a push to a branch that already existed.github.event.beforeThe all-zero rows fail outright, which is what every
ciflow/nightly/*tag push hits:The two "by accident" rows are worth spelling out, because the behavior there is correct but nothing makes it so. With
beforeabsent, the fetch becomesgit fetch --no-tags --depth=1 origin "", which quietly succeeds by fetching the default head. Then inthe unquoted command substitution word splits the empty first field away, so the script gets one argument rather than two, fails its
[ $# -eq 2 ]test, and falls through to whole tree mode. The right thing happens for the wrong reason, and only for as long as that substitution stays unquoted.Fix
Take the base as an input instead:
Empty means there is nothing to diff against. The three lint scripts already support that: given two arguments they diff a range, given none they scan the whole tree. So the empty case needs no fallback logic, it just calls them with no arguments. The all-zero SHA maps to empty, because a caller forwarding
github.event.beforehas no way to avoid producing it.With the base known up front, the separate
Fetch base refstep has nothing left to decide, so it folds into the lint step. Each job loses a step and the file gets shorter, 107 lines to 100:lint.ymlpasses the pull request base on pull requests andgithub.event.beforeotherwise, one added line.The job level
if:conditions still readgithub.event_nameand the pull request labels. Those decide whether a job runs at all, which is a different question and out of scope here. What changes is how the diff base is computed.One incidental correctness gain: the old steps checked out
inputs.refbut diffed againstgithub.shaorhead.sharead from the caller's event. Now the checked out commit and the diff head are the same input, so they cannot drift apart. That mismatch was not hypothetical, see below.What
nightly.ymlstops running, and why that is also a fixnightly.ymlnow runs the check only on the schedule and on a manual dispatch. That drops two triggers: pull requests touching.github/workflows/nightly.yml, andciflow/nightly/*tag pushes.The pull request trigger was not providing coverage.
nightly.ymlpassesref: ${{ github.sha }}, which on a pull request is the merge commit. The head commit is therefore never checked out and never fetched, sogit diff base..headcannot resolve:lint_urls.shandlint_xrefs.shconsume that diff through a process substitution, so the failure is swallowed and they report success having checked zero URLs.lint_file_size.shreads it into a plain assignment, whichset -eturns into a hard failure, and there is noskip-file-size-lintlabel to escape it. Run 31125514863 shows all three: two green jobs that examined nothing, and one red one.So this removes two no-ops and one permanently red, unskippable check. Pull requests keep the real check from
lint.yml, against their own base, on the path where the head commit is actually checked out.The tag push trigger is a policy call, not a bug fix. To be precise about causation: the exit 128 there is removed by the zero-SHA guard plus the empty default, not by the
if:. Left alone, aciflow/nightly/*push would now run a clean whole tree scan. It is skipped because there were 19 such tag pushes in the last day alone, a whole tree URL scan takes about six minutes, and it would be red on the pre-existing dead links every time.One narrow gap remains. A pull request targeting the
nightlybranch gets no link check at all, sincelint.ymlexcludes that branch throughbranches-ignore. Before this change it got the two no-ops and the red file-size job, so nothing that worked is lost. Happy to add a correctpull_requestarm tonightly.ymlif reviewers would rather close it.Result
lint.ymlmainor an existingrelease/*release/*branchciflow/nightly/*tagnightly.ymllint.ymldeclares notags:in its push trigger, so the tag case only ever reached_link_check.ymlthroughnightly.yml.Testing
runblock in_link_check.ymlis clean undershellcheck -S style.set -u, and with stray positional parameters already set, so the argument list cannot depend on how the runner invokes the step.scripts/lint_file_size.shwith no arguments to confirm the whole tree path works end to end: 9033 files checked, no failures.lint_urls.shandlint_xrefs.shusegit grep -P, which needs a git built with PCRE support that I did not have locally, so I did not run those myself. They are already exercised in CI though, by the current scheduled nightly, which reaches whole tree mode through the accident described above. Its most recentlint-urlslog checks about 2148 URLs and reports 2126 OK, 15 WARN, 7 FAIL. Those 7 are genuine dead links, unrelated to this change, and it neither fixes nor hides them, so the scheduled job stays red until they are updated.Lintruns the changed workflows against themselves and all three link check jobs pass withBASE_REFandHEAD_REFtaken from the inputs. Thenightlyrun on theciflow/nightlytag, the case that used to exit 128, is skipped.Not addressed here
lint_urls.shandlint_xrefs.shend their input pipeline with|| true, needed becausegit grepexits 1 when it finds nothing. It also means that ifgit grepfails outright the lint reports zero findings and passes. That is the same swallowing described above, it is pre-existing, and it deserves its own change.