perf(scripts): scope, batch, and deduplicate markdown link checks - #2660
perf(scripts): scope, batch, and deduplicate markdown link checks#2660Jamie Kim (jkim323) wants to merge 17 commits into
Conversation
Eval Execution |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2660 +/- ##
==========================================
- Coverage 83.17% 81.61% -1.56%
==========================================
Files 180 150 -30
Lines 32201 22141 -10060
Branches 25 0 -25
==========================================
- Hits 26782 18070 -8712
+ Misses 5416 4071 -1345
+ Partials 3 0 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Katrien De Graeve (katriendg)
left a comment
There was a problem hiding this comment.
Great one, thanks for the improvement!
Approving, but leaving one thought and suggestion for you: fall back to a full scan for deletions and renames
The changed-Markdown scope can miss broken links when a PR deletes or renames a target, such as an image, script, template, PDF, or another Markdown file, without modifying the Markdown documents that reference it.
How does this look like:
- Continue scanning only changed Markdown files for ordinary PRs.
- If the diff contains any deletion or rename, run the full Markdown link scan.
Deletions and renames are relatively uncommon and are the changes most likely to invalidate links from otherwise unchanged documents. This preserves the performance benefit for most PRs without requiring a complex reverse-reference index that could miss encoded, reference-style, or generated links.
Thanks Katrien De Graeve (@katriendg) for the thought! Verified the gap is present. The changed set is built with I'd like to propose a new suggestion, because measuring the cost changed my mind about where the scoping belongs. This PR already runs two passes with very different cost profiles
Changed-file scoping buys almost nothing on the first and nearly everything on the second. Measured on this branch, a full-repo source pass is 4.0s (1060 files, 2949 local links, 8-way parallel, no network). The external pass is the ~408 unique URLs already quoted in the PR description. Suggestion: scope by link class rather than by change scope
The reasoning is causality: a PR can break an unchanged document's internal links by deleting, renaming, or moving a target. It cannot break that document's external links — those rot on the internet's schedule, which the weekly full scan already owns. Compared with escalating on deletion or rename, this:
It fully covers the cases you listed, since image, script, template, PDF, and Markdown are all internal targets. Two pre-existing issues surfaced while measuring
What are your thoughts? |
|
Thanks Jamie Kim (@jkim323) for the new scoping and suggestions, I really like the approaches of scoping to local and external links, and batching. My recommendation is to implement both before merging:
Please ping if you need a re-review/approval. |
Pull Request
Description
The Markdown Link Check job previously revalidated the entire repository on every pull request, started one
markdown-link-checkNode process per Markdown file, and fetched the same external URL again whenever it appeared in another file.This branch applies three complementary optimizations:
Changed-file pull request scoping.
Markdown-Link-Check.ps1accepts-ChangedFilesOnlyand-BaseBranch, intersects the shared Git changed-file set with the existing discovery filters, and keeps full local scans as the default. Pull request validation checks changed Markdown files only, whileweekly-validation.ymlruns the full repository sweep.Bounded multi-file CLI batches. Markdown targets are sorted into balanced batches capped by
ThrottleLimit(default 8). Each worker invokes the pinned CLI once for multiple files, and serial JUnit conversion preserves deterministic per-file results, annotations, and fail-closed report handling.Run-scoped external URL deduplication. Source files are first checked with external requests suppressed. Exact external URLs are then partitioned into bounded synthetic aggregates, checked once through the original configuration, and replayed to every source occurrence. Query and fragment variants remain distinct, ignored URLs remain unfetched, malformed aggregate evidence fails only mapped source files, and one temporary workspace owns all derived files and cleanup.
Bot-protected host handling. The link-check configuration now ignores
https://nvd.nist.gov, which is reachable but rejects automated clients, so full local scans no longer report that host as dead.Measured impact
A network-free local benchmark measured approximately 304 ms per isolated CLI startup. Eliminating 1,106 starts saves about 5 minutes 36 seconds per full sweep from startup overhead alone, before accounting for avoided network requests and parallel execution.
Behavior preservation
npm run lint:md-linksstill performs a full local scan by default.Related Issue(s)
Fixes #2661
Fixes #2650
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
hve-builderand addressed all actionable findings.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md).github/hooks/*/*.json)evals/)Other:
.ps1,.sh,.py)Testing
npm run test:ps -- -TestPath "scripts/tests/linting/Markdown-Link-Check.Tests.ps1"git diff --checkand exact source-scope inspectionhttps://nvd.nist.govwas reported asignoredwith the updated configurationA post-fix full scan reported 0 broken links across 4,064 checked links. The exact
npm run validate:localaggregate remains blocked on Windows because the current default source batches exceed the command-line limit; this is separate from the NVD configuration fix.Coverage includes changed-file discovery, balanced batches, per-file JUnit attribution, exact URL deduplication, query and fragment variants, ignored URLs, shared dead-link replay, property-level aggregate trust, scoped aggregate failures, empty changed sets, temporary cleanup, and configs without
ignorePatterns.Checklist
Required Checks
AI Artifact Contributions
hve-builderreview mode to review contributionhve-builderreviewRequired Local Checks
The following local-safe validation commands must pass before merging:
npm run validate:localnpm run validate:docsnpm run spell-checknpm run lint:md-linksSecurity Considerations
No new dependencies were added. Workflow permissions remain
contents: read, checkout retainspersist-credentials: false, and full history is fetched only so changed-file merge-base resolution works.Additional Notes
Reviewer attention is most useful on:
aliveStatusCodestuning for bot-hostile hosts remains separate policy work requiring host-specific evidence.