Lint only what a PR adds: diff the link checks from the merge base - #21765
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21765
Note: Links to docs will display an error until the docs builds have been completed. ❌ 9 New FailuresAs of commit 9d64ebc with merge base e7ca2b2 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
5ac2e57 to
93ee0a4
Compare
There was a problem hiding this comment.
Pull request overview
Updates the link/xref/file-size linting to diff against the PR’s merge base (rather than the base branch tip), preventing pre-existing base-branch content from being misattributed to long-lived branches. This aligns local script behavior with CI and ensures CI has sufficient git history to compute merge bases.
Changes:
- Switch
lint_urls.shandlint_xrefs.shto usegit diff base...headso “diff mode” reflects changes since the merge base. - Switch
lint_file_size.shto select changed files viagit diff base...head. - Update the reusable
_link_check.ymlworkflow to fetch full history and passmerge-base(base_ref, head_ref)down to the scripts; add a regression test covering the long-lived-branch scenario.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/lint_xrefs.sh | Uses three-dot diff ranges so PR-only additions are selected relative to merge base. |
| scripts/lint_urls.sh | Uses three-dot diff ranges so URL checks only cover lines introduced on the branch. |
| scripts/lint_file_size.sh | Uses three-dot diff ranges so file-size checks target only branch-introduced changes. |
| .github/workflows/_link_check.yml | Fetches full history and computes merge base for accurate diff selection in CI. |
| .ci/scripts/tests/test_link_check_diff_selection.py | Adds regression coverage for merge-base-based diff selection behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Both the lint checks pass now: Lint / link-check / lint-urls (pull_request) |
shoumikhin
left a comment
shoumikhin
left a comment
There was a problem hiding this comment.
Went through this one carefully, including running the scripts and the new test locally.
The diagnosis is right and three dots is the correct fix. The new test fails against the pre-change scripts, with FAIL 404 on the stale URL and FAIL sub/missing.md on the xref, and passes against this branch, so it is a genuine regression test and it is wired into CI through the pytest.ini testpaths entry. Dropping --depth=1 was required rather than cosmetic: a depth-1 fetch writes a shallow graft even into an otherwise complete clone, after which git merge-base exits 1 and A...B is fatal 128. The two halves of this change are a pair, and neither works alone.
Nothing blocking. A few things worth doing before merge, two of them one-liners.
-
The
|| echo "$BASE_REF"fallback at lines 37, 70 and 103 converts a merge-base failure into a green no-op for two of the three linters. Inline comment with the measurements. -
fetch-depth: 0is unconditional, including on the callers that cannot use a merge base at all. Inline comment with the timings and a one-line gate. -
The description does not mention that this reinstates, line for line, what #17682 removed on purpose for cost. That is the one fact a reviewer needs to judge the trade, so it belongs in the description along with the measured price. Worth adding that #17682's "6 minutes to 10 seconds" was mostly the runner and Docker change rather than the fetch depth, so the restored full clone costs roughly 25 seconds of wall clock, not 6 minutes.
-
lint.yml:128sendsgithub.event.beforeasbase_refon pushes tomainandrelease/*. On a fast-forward push the merge base isbefore, so nothing changes. On a force push with diverged history it does, and the push then gets linted for content it did not change. Restricting the merge-base computation togithub.event_name == 'pull_request'would settle this and the push half of point 2 together. -
Two smaller ones.
_link_check.yml:8still documentsbase_refas "Commit to diff against", but each job now derives the diff base from it rather than using it directly. And because the checkout ishead.sha, links still resolve against the branch tree rather than the merge result, which is the mirror image of the bug being fixed here. That one is pre-existing and not worsened by this PR, and I would not change it here, but it is worth a sentence in the description as a stated limitation.
One note on rollout: the description says the fix reaches branches that are already open. That is right, but only on their next newly triggered pull-request run. A rerun keeps the original GITHUB_SHA and GITHUB_REF, and advancing the base alone does not fire synchronize, so landing this does not repair a currently red run.
| git fetch --no-tags origin "$BASE_REF" | ||
| # Diff from the merge base, not the base branch tip. Otherwise a branch cut | ||
| # before recent commits is blamed for every line it is merely missing. | ||
| args=("$(git merge-base "$BASE_REF" "$HEAD_REF" || echo "$BASE_REF")" "$HEAD_REF") |
There was a problem hiding this comment.
This fallback makes a merge-base failure silent. When git merge-base fails there is by definition no merge base, so substituting $BASE_REF hands the scripts a range that is fatal for a different reason, and the three scripts then disagree on what to do about it, for reasons of shell structure rather than git.
Running the real scripts under this workflow block with unrelated histories:
lint_urls.sh fatal: ...: no merge base rc=0 green, checked nothing
lint_xrefs.sh fatal: ...: no merge base rc=0 green, checked nothing
lint_file_size.sh fatal: ...: no merge base rc=128 red, but reported as an oversized file
lint_urls.sh:75 and lint_xrefs.sh:40 put the failing diff inside for filename in $(...) inside done < <( ... || true ), and the status is swallowed on the way out. lint_file_size.sh:44 uses a plain files=$(...) assignment under set -euo pipefail, so it exits 128 and the wrapper prints "some files exceed the 1 MB limit", which is not what happened.
Reachability, stated honestly: with fetch-depth: 0 in place there is always a merge base on the pull_request path, so this is not live today. It is reachable on a force push to main or release/* that replaces the branch with unrelated history. The bigger hazard is the latent one, that the day anyone trims the fetch depth again the two link linters go quiet instead of loud, which is exactly what #17682 did.
One line, and it matches both the existing all-zero-SHA guard here and the _get-changed-files.yml precedent of falling back to a whole-tree scan:
if MB=$(git merge-base "$BASE_REF" "$HEAD_REF"); then
args=("$MB" "$HEAD_REF")
fiAn empty args then means the whole-tree scan, which is loud and safe. Same at lines 70 and 103.
There was a problem hiding this comment.
Fixed exactly as suggested — reproduced your rc=0 / rc=0 / rc=128 first, and unset args now fall through to the whole-tree scan, verified rc=1 on a tree containing a dead link
| with: | ||
| ref: ${{ inputs.ref }} | ||
| # Enough history to find where this branch left the base branch. | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
fetch-depth: 0 is required wherever a merge base has to be computed, but it is unconditional here and several callers cannot use one: nightly.yml:34-40 passes no base_ref, workflow_dispatch has no github.event.before, a branch-creation push sends the all-zero SHA, and on a fast-forward push the merge base is github.event.before already.
From the reported job durations on this head against the base, the cost is roughly 3x per job:
this head (fetch-depth 0) lint-urls 32s lint-xrefs 36s lint-file-size 33s
base (shallow) lint-urls 12s lint-xrefs 11s lint-file-size did not run
The three jobs run concurrently, so that is about 25 seconds of added wall clock on a PR and about 79 seconds of runner time. A defensible price for correctness, just not one worth paying where it buys nothing:
fetch-depth: ${{ inputs.base_ref != '' && 0 || 1 }}Same for the two checkouts below.
For what it is worth, the cheaper alternatives do not hold up. --filter=blob:none gives a working merge base and diff but saves only about 30%, because the checkout refetches the blobs undeltified. A --deepen loop measured worse than the full clone. Fetching refs/pull/N/merge at fetch-depth: 2 is the one genuinely cheap correct option, but it changes what gets linted. Gating the full clone is the smallest thing that works.
There was a problem hiding this comment.
Gated, and pushes now keep their shallow fetch and two-dot range since the merge base there is github.event.before anyway.
| echo "Checking changed files between $base..$head" | ||
| files=$(git diff --name-only "$base..$head") | ||
| echo "Checking changed files between $base...$head" | ||
| files=$(git diff --name-only "$base...$head") |
There was a problem hiding this comment.
The other two scripts each get a regression test in this PR. This one gets the same range change with nothing pinning it, and it is the script with the worst behaviour when the range is bad: files=$(...) under set -euo pipefail propagates git's 128 straight out, and the workflow then reports a git failure as an oversized file.
A test in the same shape as the two above is about 15 lines and does not need the curl stub.
There was a problem hiding this comment.
Added, no stub needed: big.bin is oversized at the branch point and shrunk on main, so two dots picks it up from the working tree and three dots doesn't — plus a positive control that a 1MB+ file the branch actually adds still fails. Reverting the range fails the test
| # Three dots, not two. Against the base branch tip, a branch cut before recent | ||
| # commits looks like it is adding back every line those commits touched. | ||
| for filename in $(git diff --name-only --unified=0 "$1...$2"); do | ||
| git diff --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \ |
There was a problem hiding this comment.
Two small ones on the lines this PR is already editing.
Neither git diff here passes --no-color, while the whole-tree branch at :83 uses git grep --no-color. With color.ui = always in a user's config the diff branch silently stops matching, since the + lines arrive wrapped in escape sequences.
Also, only the file selection on the line above is actually pinned by the new test. Reverting just this inner ... back to .. leaves both tests green, because the selection above has already excluded stale.md by then and feature.md reads as added under either range. If you want the test to hold both occurrences, one fixture where a file is modified on both sides would do it.
Same two points apply to lint_xrefs.sh.
There was a problem hiding this comment.
Both fixed: --no-color on each diff with a color.ui = always test (2 matches → 0 without it), and a both_sides.md fixture edited on both branches — reverting just the inner range now fails 3 tests
| def setUp(self): | ||
| self.repo = Path(tempfile.mkdtemp()) | ||
| self.addCleanup(shutil.rmtree, self.repo, ignore_errors=True) | ||
| self.git("init", "-q") |
There was a problem hiding this comment.
Small robustness note, since this repo is checked out on developer machines with all sorts of config. commit.gpgsign, init.defaultBranch, core.autocrlf and a global .gitattributes are already defended here, but core.hooksPath, init.templateDir, core.excludesFile and inherited GIT_DIR / GIT_WORK_TREE still break this fixture. Clearing those in the subprocess env in git() closes the whole class in one line.
There was a problem hiding this comment.
Done, slightly wider: GIT_CONFIG_GLOBAL/SYSTEM=/dev/null plus stripping GIT_* from the child env closes your four in one line — and autocrlf/.gitattributes too, which weren't actually defended before.
93ee0a4 to
3866c55
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (5)
.github/workflows/_link_check.yml:90
- Same issue as above: if
git merge-basefails,argsremains empty andlint_xrefs.shwill run a whole-tree scan instead of being scoped to PR changes.
# lacks. No merge base means no usable range, so leave args empty for a
# whole tree scan: the scripts treat a bad range as nothing to check.
if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then
args=("$merge_base" "$HEAD_REF")
fi
.github/workflows/_link_check.yml:134
- Same issue as above: if
git merge-basefails,argsremains empty andlint_file_size.shwill run a whole-tree scan instead of being scoped to PR changes.
# lacks. No merge base means no usable range, so leave args empty for a
# whole tree scan: the scripts treat a bad range as nothing to check.
if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then
args=("$merge_base" "$HEAD_REF")
fi
.github/workflows/_link_check.yml:46
- If
git merge-basefails here,argsstays empty and the scripts will fall back to a whole-tree scan (theirelsepath), which defeats the PR-only intent. The comment also says a bad range is treated as “nothing to check”, but leavingargsempty does the opposite.
This issue also appears in the following locations of the same file:
- line 86
- line 130
# lacks. No merge base means no usable range, so leave args empty for a
# whole tree scan: the scripts treat a bad range as nothing to check.
if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then
args=("$merge_base" "$HEAD_REF")
fi
scripts/lint_urls.sh:75
- The
--name-onlydiff doesn’t apply theexcludespathspecs, so excluded paths are still enumerated and looped over (even though the per-file diff later excludes them). Applying the excludes to the file list reduces unnecessary work on large diffs.
for filename in $(git diff --no-color --name-only --unified=0 "$1...$2"); do
scripts/lint_xrefs.sh:40
- The
--name-onlydiff doesn’t apply theexcludespathspecs, so excluded paths are still enumerated and looped over (even though the per-file diff later excludes them). Applying the excludes to the file list reduces unnecessary work on large diffs.
for filename in $(git diff --no-color --name-only --unified=0 "$1...$2"); do
The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. pytorch#21729 failed exactly that way, on nine links pytorch#21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what pytorch#17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5).
3866c55 to
9d64ebc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
.github/workflows/_link_check.yml:78
- Same as lint-urls: because this workflow runs under
workflow_call,github.event_nameisworkflow_call, sofetch-depthhere will always be'1'andFROM_MERGE_BASEwill always befalse. That prevents merge-base resolution on PRs and can leave the scripts with insufficient history to computeA...Bdiffs reliably.
- uses: actions/checkout@v4
with:
ref: ${{ inputs.ref }}
# Only the merge base path needs history, and it costs ~25s. Quote the
# branches: bare 0 is falsy, so `&& 0 || 1` would always yield 1.
fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }}
- name: Lint cross-references
env:
BASE_REF: ${{ inputs.base_ref }}
HEAD_REF: ${{ inputs.ref }}
FROM_MERGE_BASE: ${{ github.event_name == 'pull_request' }}
run: |
.github/workflows/_link_check.yml:122
- Same event-name issue applies here:
fetch-depth/FROM_MERGE_BASEkeyed offgithub.event_name == 'pull_request'will never be true underworkflow_call. Also note the job-levelif: ${{ github.event_name == 'pull_request' }}above this step will evaluate false in a called workflow, so this entire job is likely skipped in all contexts.
- uses: actions/checkout@v4
with:
ref: ${{ inputs.ref }}
# Only the merge base path needs history, and it costs ~25s. Quote the
# branches: bare 0 is falsy, so `&& 0 || 1` would always yield 1.
fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }}
- name: Lint file sizes
env:
BASE_REF: ${{ inputs.base_ref }}
HEAD_REF: ${{ inputs.ref }}
FROM_MERGE_BASE: ${{ github.event_name == 'pull_request' }}
run: |
Thanks — all seven done, pushed. |
|
All the relates jobs pass |
|
Also checked with Anthony and he verbally agreed with the latest changes / fixes based off his review comments . Merging this now |
Summary
The URL, xref, and file-size linters diffed
base..head, wherebaseis the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions, and the branch gets blamed for links someone else already repaired.#21729 failed exactly this way. It touches 18 files and adds no URLs at all, but the two-dot diff scoped the lint to 143 files and flagged nine links that #21694 had already fixed or
@lint-ignored. #21707 is starker: one Python file with no URLs in it, 199 files linted, the same nine failures.The stray
jq: parse errorlines in #21729's log are the same symptom from the other direction: the job runs the branch's own pre-#21694 copy oflint_urls.sh.Fix
The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a
pull_requestthe reusable workflow resolves from the merge commit, so it carries this fix even thoughscripts/still comes from the branch itself. The scripts also switch to three-dot ranges so./scripts/lint_urls.sh main HEADby hand behaves the same.lint_xrefs.shandlint_file_size.shhad the identical bug and get the identical change.Cost, stated plainly. A merge base needs real history, so this restores
fetch-depth: 0on thepull_requestpath — line for line what #17682 removed in February for speed. Measured on this branch, that is ~25s of added wall clock and ~79s of runner time across the three concurrent jobs. #17682's 6min → 10s was mostly the runner and Docker change rather than the fetch depth, though the commit changes both at once and I can't fully separate them. Pushes and the nightly whole-tree scan cannot use a merge base and stay shallow:The quotes matter — bare
0is falsy in GitHub expressions, so&& 0 || 1always yields1and would silently disable the fix.No silent fallback. When there is no merge base there is no usable range, so the range is left unset and the linters scan the whole tree. Substituting the base tip instead produces a range the scripts fail on quietly: verified against unrelated histories,
lint_urls.shandlint_xrefs.shboth exit 0 having checked nothing, whilelint_file_size.shexits 128 and the wrapper reports "some files exceed the 1 MB limit", which is not what happened. Unset args are loud instead — verified rc=1 on a tree containing a dead link.--no-color. Bothgit diffcalls now pass it, matching thegit grep --no-colortwo lines below. Withcolor.ui = alwaysin a developer's config, added lines arrive wrapped in escape sequences,grep -E '^\+'matches nothing, and the check passes having found nothing (measured: 2 matches → 0).Test plan
.ci/scripts/tests/test_link_check_diff_selection.pybuilds a diverged history wheremainrepairs bad links and shrinks an oversized file while the feature branch simply predates all of it. Four fixture files each pin a different part, andcurlis stubbed so there is no network:both_sides.md— edited on both branchesbig.bin— oversized at the branch point, shrunk on mainlint_file_size.sh's rangecolorful.gitconfig—color.ui = always--no-colorbase_only.md/feature_only.mdMutating each changed line individually:
The file-selection range is not pinned because it cannot be: with the per-file diff at three dots, the extra files it selects produce empty diffs. Replayed against #21707's real 199-file range, both variants emit byte-identical output. It is changed for consistency, not behaviour.
Narrowing the scope must not blunt the check, so each linter also has a positive control where the branch itself adds the bad thing:
End to end on real content: a synthetic commit on top of
mainthat puts #21694's nine repaired links back, as if a PR had added them, givesrc=1with 9 FAIL and 5 OK against the live network. Incidentallymusl.ccanswered this time where CI saw000, andpybind/cmake_examplenow 404s where CI saw301— which is the retry-then-WARN path earning its keep.Also replayed #21729's and #21707's exact CI refs through the fixed scripts (exit 0 each), and ran all three linters plus
lintrunneragainst this PR's own diff.Known limitations
head.sha, so references still resolve against the branch tree rather than the merge result — the mirror image of the bug fixed here. Pre-existing and not worsened by this PR.git grepfailure and exits 0, so on a git built without PCRE the scan silently checks nothing. Reachable locally, not on the runners, which is why the nightly scan works. Pre-existing; worth a follow-up rather than widening this PR.Rollout
This does not repair a currently red run. A rerun keeps the original
GITHUB_SHA, and advancing the base alone does not firesynchronize, so an already-open PR picks the fix up only on its next newly triggered pull-request run — any push, or close/reopen. That is still cheaper than a content rebase: no history rewrite, no conflicts, nothing to re-review.Supersedes #21762, which fixes the same bug but leaves the checkout shallow and misses
lint_urls.sh. Its reviewer's shallow-checkout point is exactly right: a--depth=1fetch writes a shallow graft even into an otherwise complete clone, after whichgit merge-basefails andA...Bis fatal, so the two halves of this change are a pair. The regression test here is adapted from that PR.Authored with Claude Code (Claude Opus 5).