fix(ci): scope markdown-link-check to the PR's merge base - #3620
fix(ci): scope markdown-link-check to the PR's merge base#3620liamcrumm wants to merge 1 commit into
Conversation
`check-modified-files-only` runs `git diff --name-only <base-branch>`, a two-dot diff against the base tip. On a branch that has fallen behind main, every commit that landed on main since the branch point is reported as modified, so the job checks files the PR never touched and fails on links that were already fixed upstream. Measured on #3450 (55 commits behind, one changed .md file): the job checked 255 markdown files and failed on four links that #3352 had already fixed on main. All ten PRs currently failing this check are behind main, by 14 to 269 commits. Resolve the base to its merge base with HEAD so the diff covers only what the branch actually changed. Non-pull-request events keep the previous behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: liamcrumm <14815298+liamcrumm@users.noreply.github.com>
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
TL;DR: 0 blockers, 0 warnings. No issues found. Clean change.
This PR fixes CI behavior for docs-only PRs by scoping markdown-link-check to the PR’s merge base rather than the base branch tip, avoiding false failures when a PR branch is behind main.
Changes:
- Adds a step to compute
git merge-base origin/$BASE_REF HEADfor pull requests and feeds that SHA intobase-branch. - Preserves existing behavior for non-PR events (schedule/push), with a warning + fallback when merge-base cannot be resolved.
|
Validated the scoping change locally against #3450 (55 commits behind, one changed markdown file): Note this PR cannot exercise the change itself: |
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
This is right, and it has been sitting for eight days blocked on a single word.
The diagnosis matches what #3530 fixed for the spell-check job. check-modified-files-only runs git diff --name-only <base-branch>, a two-dot diff against the base tip, so a branch behind main reports every commit that landed since the branch point as modified. The job then evaluates those files as they exist on the stale branch and fails on links that were fixed upstream, which is a failure attributed to a contributor for something they did not touch and cannot fix. #3450 at 55 commits behind is a fair illustration.
Resolving to the merge base is the correct scope. Two details I would call out as done right rather than incidental: falling back to the base tip with a ::warning:: annotation rather than failing, so an unresolvable merge base degrades to the old over-reporting behaviour instead of blocking; and keeping the previous behaviour when github.base_ref is empty, so schedule and push triggers are unaffected.
The only thing blocking this is a dictionary entry, and it is in someone else's PR.
Spell-check changed files fails twice on linkbase, the step id this PR introduces. That word is not in .cspell-repo-terms.txt on main. It is in my #3640, added with the comment "Step id in ci.yml's markdown-link-check merge-base scoping (#3620)". So the two PRs are interlocked: this one cannot go green until that word lands, and nothing about that is visible from either PR's checks. Whoever picks either up should take both, or move the entry across.
One correction I owe this PR. #3640's description claimed this one inherits a stale-ref flaw, on the theory that actions/checkout narrows remote.origin.fetch so git fetch origin <branch> leaves origin/<base> stale. MohammadHaroonAbuomar showed that does not hold on current main, and I confirmed it. That criticism is withdrawn: git fetch --no-tags origin "$BASE_REF" followed by git merge-base is fine as written here, and I have rewritten #3640 to stop saying otherwise.
No approve bit on this repo, so a comment, but I would merge it.
Summary
Scope
markdown-link-checkto the files a PR actually changed, instead of every file that changed onmainsince the branch point.Problem
The job sets
check-modified-files-only: 'yes'withbase-branch: main. That action runsgit diff --name-only main, a two-dot diff against the base tip, so on a branch that has fallen behind, every commit that landed onmainsince the branch point is reported as modified.The job then checks those files as they exist on the stale branch, and fails on links that were already fixed upstream.
Measured on #3450, which is 55 commits behind and changes exactly one markdown file:
It failed on four links, all of which #3352 had already fixed on
mainbefore that run executed.Every PR currently failing this check is behind
main:The job's own comment states the intent ("Only check links in files changed by the PR/push to avoid pre-existing broken links blocking unrelated work"), which the two-dot diff defeats.
ci-completelistsmarkdown-link-checkin itsneeds, so each of these also failsci-complete.Changes
.github/workflows/ci.ymlbase-branchtogit merge-base origin/$BASE_REF HEADbefore invoking the actionNon-pull-request events (schedule, push) keep the previous behaviour. If
merge-basecannot be resolved, the step warns and falls back to the base tip, which is exactly today's behaviour.Testing
python3 scripts/ci/generate_workflows.py --checkpasses, and the YAML parses withyaml.safe_load. This PR's ownmarkdown-link-checkrun exercises the change.