Skip to content

ci: validate repository links before merge - #3118

Draft
rwgk wants to merge 1 commit into
rwgk/lychee-portable-pre-commitfrom
rwgk/lychee-check-checkout-links
Draft

rwgk wants to merge 1 commit into
rwgk/lychee-portable-pre-commitfrom
rwgk/lychee-check-checkout-links

Conversation

@rwgk

@rwgk rwgk commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Lychee currently checks repository links against remote main. When a PR adds or moves a path and updates documentation to link to its new location on main, that target does not exist yet. This creates a chicken-and-egg problem: the link check needs the PR to merge before it can pass, while the PR needs a passing link check before merge.

For example, #3116 renames cuda_bindings/ to cuda_bindings_13/. Its intended repository link is:

https://github.com/NVIDIA/cuda-python/tree/main/cuda_bindings_13

That link should be checked against the proposed directory layout before it reaches main.

Solution

Builds on #3135, which extracts the native hook installation, launcher, and Windows contributor guidance for separate review.

Validate these repository links against the checkout being tested, while keeping the intended main URLs in source files and published documentation.

  • CI: The dedicated link-check action invokes lycheeverse/lychee-action directly, using Lychee's native --remap to change the validation target to the actual checked-out commit in the CI repository. Authored and rendered documentation checks, including retries, use the same remapping. GitHub's file and directory views are preserved, so rendered checks retain their existing fragment validation.
  • pre-commit: Extend the portable native hook from ci: make the Lychee pre-commit hook portable on Windows #3135 with anchored --remap rules for the working tree. This also covers uncommitted or unpushed paths. Source files are not rewritten, and local checks retain their existing policy of not checking fragments. CI exercises the configured hook against bounded local fixtures on Linux and Windows; full documentation link scans run through the dedicated CI action.

For the example above, CI checks tree/<checked-commit>/cuda_bindings_13; pre-commit checks the checkout's cuda_bindings_13/ directory. Missing targets still fail.

Scope

Remapping applies only to canonical https://github.com/NVIDIA/cuda-python/blob/main/ and tree/main/ links. Links to tags, explicit commits, other branches, and other repositories keep their existing targets.

These rules validate the proposed main tree. Maintenance branches do not currently contain this action; adopting it there requires an explicit policy for links that intentionally point to main rather than the maintenance checkout.

This PR prepares the link-checking infrastructure for #3116. The source-directory rename remains in that separate PR.

Validation

After extraction: 246 CI-tool tests plus 36 subtests, full pre-commit, and the installed-hook smoke checks pass locally. Hosted CI is deferred for web review; the rewritten commit includes [skip ci].

Earlier validation before this extraction:

@rwgk rwgk added this to the cuda.bindings next milestone Oct 15, 2026
@rwgk rwgk added enhancement Any code-related improvements CI/CD CI/CD infrastructure labels Oct 15, 2026
@copy-pr-bot

copy-pr-bot Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rwgk rwgk self-assigned this Oct 15, 2026
@rwgk
rwgk added this pull request to stack #3119 October 15, 2026 06:20

rwgk commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 8934e74

@github-actions

Copy link
Copy Markdown
Contributor

@rwgk
rwgk marked this pull request as ready for review October 15, 2026 15:29
@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 94ce501b-1f10-44df-89ee-764257651bd8
📥 Commits

Reviewing files that changed from the base of the PR and between 1f41281 and 8934e74.

📒 Files selected for processing: 6
  • .github/actions/check-doc-links/action.yml
  • .pre-commit-config.yaml
  • ci/README.md
  • ci/tools/preprocess_repo_links.py
  • ci/tools/tests/test_preprocess_repo_links.py
  • ci/tools/tests/test_repo_link_check_action.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Repository links to files added or moved in the same pull request are now checked against the current checkout, both in CI and with local pre-commit checks.
    • Missing link targets continue to fail validation; unrelated links are checked as before.
  • Documentation
    • Added guidance on how repository links are validated in CI and locally.

The link-checking setup now resolves canonical blob/main and tree/main links against local checkout content during pre-commit checks and against the checked-out commit during CI checks.

Changes: Repository link validation

Layer / File(s) Summary
Local link preprocessing
ci/tools/preprocess_repo_links.py, ci/tools/tests/test_preprocess_repo_links.py, .pre-commit-config.yaml, ci/README.md
The preprocessing script converts matching repository links to file URLs rooted at the checkout. The pre-commit hook runs the script, and tests cover URL formats, preserved content, and CLI errors. The README documents local preprocessing and CI link checks.
CI commit-specific remaps
.github/actions/check-doc-links/action.yml, ci/tools/tests/test_repo_link_check_action.py
The action resolves the checkout commit and remaps canonical blob/main and tree/main links to matching views in the current repository at that commit. The integration test checks the generated remaps and their boundaries.

Priority: ⬇️ Low

Change: Feature

Merge Risk: ⚪ Minimal · up to 8934e

This change lets repository links be validated against the checkout under test. It affects only CI and local link checks, not runtime behavior, and no merge-blocking risk was identified.

- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juenglin

Copy link
Copy Markdown
Contributor

Has the lychee pre-commit step been tested on both Windows and Linux?

@juenglin juenglin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice approach. The --remap quoting is right: the embedded \" survive eval in the lychee-action entrypoint and shlex.split in retry_lychee.py, so both passes give lychee one "pattern replacement" argument per --remap. The 16 new tests pass for me locally. Nothing blocking, but a few things to consider:

1. Windows pre-commit (follow-up to @juenglin's question)
CI skips lychee in pre-commit on both Linux and Windows (SKIP: lychee), so nothing exercises --preprocess=./ci/tools/preprocess_repo_links.py on Windows. Lychee launches that path directly rather than through a shell, so it depends on the shebang and exec bit. Windows doesn't honor shebangs. I haven't verified how lychee handles this on Windows. Has anyone run the hook there? If Windows local runs are supported, it may need an explicit interpreter or a note in ci/README.md.

2. Backport-branch PRs
check-doc-links runs for every pull-request/N ref, not just PRs against main. For a PR against 12.9.x, 13.4.x, 12.2.x or 11.8.x, HEAD is the backport commit. A tree/main/<path> link is then checked against that branch, so a path that exists only on main (for example cuda_bindings_13 after #3116) would fail. A path that was removed from main but still exists on the branch would pass. I haven't checked whether those branches contain such links today. Is it worth gating the remap on the base branch, or documenting the behavior in ci/README.md?

3. CI and local patterns differ
CI uses an anchored regex (^https://github[.]com/...). preprocess_repo_links.py does an unanchored str.replace, so it would also rewrite the prefix inside another URL, such as a redirect query string. This doesn't happen in today's docs. It might be worth a comment, or defining the prefixes once so the two paths can't drift. (I considered using --remap locally too, but pre-commit args are static and the checkout root isn't, so --preprocess is the right mechanism.)

4. Smaller notes

  • The remapped URLs embed the commit SHA, and lychee keys its cache on the remapped URI. So the nightly baseline cache can't serve these links on PRs, and about 21 authored links (more in rendered HTML) are refetched on every run. That is probably fine for the rate-limit budget, but it's worth knowing.
  • test_repo_link_check_action.py and test_preprocess_repo_links.py run only in the nightly ci/tools/tests job. Only test_compute_ci_plan.py runs per PR. Consider adding the action test to ci.yml, since it's the one most likely to catch breakage in the remap step.
  • The description's "missing targets still fail" and "17 regression probes" aren't backed by anything in the diff. If those probes are cheap to keep, a Lychee-driven fixture test would cover them.

@rwgk

rwgk commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author
Tracking a codex analysis & recommendations at this stage

I fetched the latest feedback and read the Slack thread. I agree with adding limited Lychee coverage to pre-commit CI on Linux and Windows. The review exposes a Windows integration risk that I would address before merging #3118.

Windows: the history matters

Mike’s original PR was #2327, “Make pre-commit work on Windows”.

Its initial description says Lychee does not run on Windows, but the discussion refined that conclusion: Lychee itself supports Windows, and Mike confirmed that the installed Git hook works. Git Bash was also documented as a supported approach.

That gives us existing Windows behavior to preserve.

PR #3118 introduces a separate problem: Lychee launches the Python preprocessor directly, without an interpreter or shell. The pinned implementation uses Command::new(...).arg(path). Native Windows execution does not interpret the script’s shebang.

My source-based conclusion is that the new preprocessing command is likely broken on Windows, including when the outer hook starts successfully through Git Bash. I have not reproduced this on Windows.

Our earlier validation established Linux behavior. Windows CI skipped Lychee, and the Python tests launch the helper through sys.executable, bypassing this particular failure.

The small CI check I recommend

Add a separate step to the existing Linux and Windows pre-commit jobs that invokes the actual configured Lychee hook, with Lychee removed from SKIP for that step.

Generate tiny Markdown and reStructuredText fixtures during the job, and verify:

  • Canonical blob/main/ and tree/main/ links resolve to checkout-only files and directories.
  • Missing targets produce the expected link failure.
  • Input files remain unchanged.

Generating fixtures avoids committing intentionally broken links that nightly checks would discover. With no external links, this should be quick and deterministic after installation.

I would also run the new Python tests on every PR. Currently they run automatically only in the nightly CI-tools suite.

Mike’s follow-up #2729 adds another useful lesson: cached environments hid an installation failure. Changes to the hook’s installation or pin should therefore receive a fresh-install check too.

A promising implementation simplification

I would investigate a small Python hook wrapper that computes the checkout path and passes dynamic, anchored --remap arguments to Lychee.

That could resolve both the Windows subprocess problem and the reviewer’s concern about unanchored text replacement. The official lychee-bin==0.24.2 distribution has Windows wheels, so this need not involve writing another binary installer. It would still require validation through the real hook on both platforms.

Would the nightly cache make broader coverage quick?

It should help substantially with unchanged external URLs, but reuse needs explicit wiring.

The current pre-commit jobs restore hook environments; they do not restore the nightly Lychee response cache. A broader Linux hook run could reuse the nightly authored cache. Windows reuse would require additional cache portability work.

There are also two expected limits:

For #3118, I favor the small fixture check on both platforms. Broader cached documentation coverage can follow without making this fix depend on external-site reliability.

The remaining feedback

Ralf’s review supports the approach and raises useful refinements:

Point My assessment
Maintenance branches A policy issue to clarify. None of the four named branches currently contains this action, so merging #3118 does not introduce that behavior there. Document the main-development scope and decide explicitly before adopting it on maintenance branches.
Anchored CI matching versus local replacement A real mismatch. Local replacement can modify a GitHub URL embedded inside another URL. Native anchored remapping could resolve this alongside the Windows fix.
“17 regression probes” These were ad-hoc validation, not committed regression coverage. The proposed fixture checks would preserve the essential guarantees in CI.

My recommended scope for #3118 is: fix Windows invocation, align URL matching, add small real-hook checks on both platforms, enable the focused Python tests per PR, and clarify branch scope. That keeps the work focused while closing the gaps identified by review.

@rwgk
rwgk marked this pull request as draft October 15, 2026 18:28

rwgk commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test cc6fa67

rwgk added a commit that referenced this pull request Oct 15, 2026
Rename the CUDA 13 bindings source root and update its build, test, CI,
documentation, and tooling paths. This is the same file tree as the
validated review branch, with linear history above PR #3118.

Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
rwgk added a commit that referenced this pull request Oct 15, 2026
Rename the CUDA 13 bindings source root and update its build, test, CI,
documentation, and tooling paths. This is the same file tree as the
validated review branch, with linear history above PR #3118.

Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@rwgk
rwgk force-pushed the rwgk/lychee-check-checkout-links branch from cc6fa67 to 200be8b Compare October 15, 2026 20:00
@rwgk
rwgk removed this pull request from stack #3119 October 16, 2026 05:34
@rwgk
rwgk force-pushed the rwgk/lychee-check-checkout-links branch from 0bed5a9 to 3019f24 Compare October 16, 2026 05:57
@rwgk
rwgk added this pull request to stack #3131 October 16, 2026 05:57
@rwgk
rwgk force-pushed the rwgk/lychee-check-checkout-links branch 2 times, most recently from 5d7f177 to de7c722 Compare October 16, 2026 15:24
Build on the portable native Lychee hook to remap canonical main source links to the checkout or exact CI commit. Preserve source URLs, check missing targets, and retain the existing Linux/Windows hook smoke coverage and rendered-fragment policy.

Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
@rwgk
rwgk force-pushed the rwgk/lychee-check-checkout-links branch from de7c722 to fad2ab1 Compare October 16, 2026 15:51
@rwgk
rwgk removed this pull request from stack #3131 October 16, 2026 15:52
@rwgk
rwgk changed the base branch from main to rwgk/lychee-portable-pre-commit October 16, 2026 15:52
@rwgk
rwgk added this pull request to stack #3136 October 16, 2026 15:52

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD infrastructure enhancement Any code-related improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants