Repository navigation
ci: make Lychee validate repository links against the checkout - #3143
Conversation
|
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
added
enhancement
ChatGPT Codex Connector
280c425 to
672e513
Compare
|
/ok to test 672e513 |
|
672e513 to
d24e19f
Compare
d24e19f to
983ac75
Compare
rwgk
changed the title
rwgk
added
the
PR review get-together
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ⬇️ Low Change: Feature Merge Risk: ⚪ Minimal · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments: 1
ci/tools/run_lychee.py: 1
66-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Exercise nonempty remaps at the subprocess boundary.
The
run_lychee.main()tests replacerepo_link_remapswith[], so they do not detect a regression that drops remap options beforesubprocess.run. The helper tests cover remap construction separately, but not this caller wiring. Add nonempty remaps to both authored and rendered tests and assert their exact position in the Lychee command.Suggested fix
- patch("run_lychee.repo_link_remaps", return_value=[]), + patch("run_lychee.repo_link_remaps", return_value=["--remap", "source=target"]), ... - assert command[1:4] == ["--format", "json", "--output"] - report = Path(command[4]) + assert command[1:3] == ["--remap", "source=target"] + assert command[3:6] == ["--format", "json", "--output"] + report = Path(command[5]) ... - patch("run_lychee.repo_link_remaps", return_value=[]), + patch("run_lychee.repo_link_remaps", return_value=["--remap", "source=target"]), ... [ str(tmp_path / "lychee"), + "--remap", + "source=target", "--files-from",
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cuda-python/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
13a83dd1-9c60-4b9d-84a2-a179372b52cb
📒 Files selected for processing: 9
.github/workflows/build-docs.yml.github/workflows/lychee.yml.github/workflows/pre-commit.ymlci/README.mdci/tools/checkout_link_defaults.pyci/tools/lychee_cache.pyci/tools/run_lychee.pyci/tools/tests/test_checkout_link_defaults.pyci/tools/tests/test_run_lychee.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| # BEGIN SYNCED LYCHEE REPOSITORY METADATA | ||
| env: | ||
| LYCHEE_REPOSITORY: ${{ github.repository }} | ||
| LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
fd -e yml -e yaml . .github/workflows --exec sh -c 'echo "== $1"; sed -n "1,45p" "$1" | rg -n "on:|schedule|cron|workflow_call|workflow_dispatch|push|pull_request|uses: ./.github/workflows/(lychee|pre-commit)" ' _ {}Repository: NVIDIA/cuda-python
Length of output: 6774
🏁 Script executed:
set -e
printf '%s\n' '== workflow triggers and caller references =='
for f in .github/workflows/lychee.yml .github/workflows/pre-commit.yml .github/workflows/ci-nightly.yml .github/workflows/ci.yml .github/workflows/build-docs.yml; do
echo "== $f"
nl -ba "$f" | sed -n '1,180p'
done
printf '%s\n' '== relevant helper and prepare_ci references =='
rg -n -F --glob '*.py' -e 'def _branch' -e 'LYCHEE_DEFAULT_BRANCH' -e 'prepare_ci' ciRepository: NVIDIA/cuda-python
Length of output: 41037
🏁 Script executed:
set -e
nl -ba ci/tools/checkout_link_defaults.py | sed -n '35,205p'Repository: NVIDIA/cuda-python
Length of output: 8299
🌐 Web query:
GitHub Actions schedule event payload empty github.event.repository default_branch reusable workflow github context caller
💡 Result:
Yes—**a `schedule` event has no webhook payload**, so `github.event.repository.default_branch` is not available. GitHub’s event reference lists the schedule payload as “Not applicable”; the event object isn’t a repository object. ([github.com](https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md?utm_source=openai))
In a reusable workflow called by that scheduled workflow, **`github` still refers to the caller’s context**. So `github.event.repository.default_branch` remains unavailable there; calling a reusable workflow doesn’t add the missing payload fields. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai))
Use the caller’s `github.repository` to identify the repo and, if you need its default branch, query repository metadata (for example, with the GitHub API). The event’s `github.ref` is the default branch ref for a scheduled run, but it is not itself the `default_branch` property. ([github.com](https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md?utm_source=openai))
Citations:
- 1: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md?utm_source=openai
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/reusing-workflow-configurations?utm_source=openai
- 3: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/events-that-trigger-workflows.md?utm_source=openai
🏁 Script executed:
set -e
printf '%s\n' '== workflow references =='
rg -n -F --glob '*.yml' --glob '*.yaml' -e './.github/workflows/lychee.yml' -e './.github/workflows/pre-commit.yml' -e './.github/workflows/build-docs.yml' .github/workflows
printf '%s\n' '== metadata blocks =='
for f in .github/workflows/lychee.yml .github/workflows/pre-commit.yml .github/workflows/build-docs.yml; do
echo "== $f"
rg -n -C 8 -F 'LYCHEE_DEFAULT_BRANCH:' "$f"
doneRepository: NVIDIA/cuda-python
Length of output: 3010
important: Add a schedule fallback for the default branch.
ci-nightly.yml runs on schedule and calls lychee.yml. Scheduled events do not provide github.event.repository.default_branch, so the helper receives an empty branch and fails before the nightly link checks run.
pre-commit.yml is not scheduled, and build-docs.yml guards this step to pull-request refs. Apply the same fallback to all synced blocks:
Suggested fix
--- a/.github/workflows/lychee.yml
+++ b/.github/workflows/lychee.yml
@@
- LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
+ LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch || (github.event_name == 'schedule' && github.ref_name) || '' }}
--- a/.github/workflows/pre-commit.yml
+++ b/.github/workflows/pre-commit.yml
@@
- LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
+ LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch || (github.event_name == 'schedule' && github.ref_name) || '' }}
--- a/.github/workflows/build-docs.yml
+++ b/.github/workflows/build-docs.yml
@@
- LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
+ LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch || (github.event_name == 'schedule' && github.ref_name) || '' }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} | |
| LYCHEE_DEFAULT_BRANCH: ${{ github.event.repository.default_branch || (github.event_name == 'schedule' && github.ref_name) || '' }} |
Source: Path instructions
983ac75 to
1e5ecd1
Compare
1e5ecd1 to
6be5142
Compare
6be5142 to
b361255
Compare
|
Thanks for the stack. Two issues to fix before merge: 1. Nightly will fail at "Discover repository link targets". This is the open CodeRabbit thread on 2. The local hook hard-fails when the remote HEAD is not cached. |
5010c1e to
a7d8e47
Compare
Great observation. It took me a few minutes to unpack this finding. The issue escaped my attention before (I naively assumed it is easy to determine the default branch). I'm taking a step back now to look at this carefully. |
codex GPT-6.1-Sol ultra: Commit 911747a simplifies the checkout-aware link handling. The key observation is that our public The resulting policy is:
This removes the three CI discovery steps and the local cached remote-HEAD requirement. Checking builds also select an appropriate reference for generated example links. Tags, explicit commits, other branches, and unrelated repository links retain their original targets. Validation passes: 259 CI-tool tests, 36 subtests, and all-files pre-commit. Native Lychee probes also confirm that missing local targets fail despite an earlier cached success, and that ordinary HTML anchor failures remain detectable.
This is moot now, too: commit 911747a removes “Discover repository link targets” from all three workflows, including nightly. |
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
911747a to
1d24145
Compare
|
/ok to test 1d24145 |
|
🤖 Independent review by Claude Code (Claude Fable 5.1), requested by the author after the post-approval simplification. Covers the branch at 74fc792 (commits 1d24145 and 74fc792) against Verdict: no blocking issues. Mergeable once the draft is marked ready, optionally after picking up the one CodeRabbit nit that still applies. Findings (all low severity)
Nits: a required CodeRabbit findings
What was verified
Runner work versus Probe commands (lychee-bin 0.24.2)ROOT_URI=$(python -c 'from pathlib import Path; print(Path(".").resolve().as_uri() + "/")')
# Authored mode, as emitted by repo_link_remaps(ROOT). Run twice with a linked
# file deleted in between: the second run fails; .lycheecache stays empty for file targets.
lychee --offline -vv --cache --remap "^https://github[.]com/NVIDIA/cuda-python/(blob|tree)/main/ ${ROOT_URI}" authored.md
# Local rendered mode: fragments dropped only on the remapped source links.
lychee --offline -vv --include-fragments=full \
--remap "^https://github[.]com/NVIDIA/cuda-python/(?:blob|tree)/main/([^#]*)(?:#.*)?$ ${ROOT_URI}\$1" index.html
# Public rendered mode at a real commit; fragments checked on GitHub's blob pages.
lychee -vv --include-fragments=full \
--remap "^https://github[.]com/NVIDIA/cuda-python/(blob|tree)/main/ https://github.com/NVIDIA/cuda-python/\$1/$(git rev-parse upstream/main)/" pub.htmlThe |
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
Signed-off-by: Ralf W. Grosse-Kunstleve <rgrossekunst@nvidia.com>
|
codex GPT-6.1-Sol ultra: Thanks for the detailed Fable review. The two actionable findings are addressed in separate commits:
Local validation passes: all 259 CI-tool tests and 36 subtests, plus |
|
codex GPT-6.1-Sol ultra: Regarding the Fable review's cache observation: rendered source links are checked at the proposed commit, so their URLs usually differ from the URLs for the default-branch commit in the nightly baseline. A small number of fresh GitHub requests is an accepted cost of validating the proposed source tree, including its GitHub source-view fragments. A success for a different commit would not establish that the proposed target is valid. This does not add a job or documentation build. Authored source links become local file checks, reducing requests there, and established external links continue to use the shared cache. Matching SHA URLs can still reuse cached successes, including during retries; the review's “never” is therefore slightly stronger than the actual behavior. The current link inventory is useful context rather than an exact request-count guarantee. |
|
codex GPT-6.1-Sol ultra: For completeness, the remaining review observations do not require implementation changes:
The wrapper-test concern from the same CodeRabbit review is addressed by the new test commit described above. |
|
/ok to test dd3100c |
rwgk
removed
the
PR review get-together
2 participants
Highlights
Let Lychee validate public
mainsource links against the proposed checkout, so a path and its documentation can land in the same PR.main.This builds on #3155 and provides the checkout-aware link handling needed by #3147.
How it works
The helper recognizes only these complete public URL prefixes:
The public URL convention is explicit. There is no default-branch discovery, cached remote
HEADrequirement, or additional public remote to configure.Scope and tradeoffs
Version tags, explicit commits, maintenance branches, unrelated repositories, and URLs embedded inside other URLs retain their original targets. A future change to the public
mainURL convention would require an explicit matching-rule update.Public rendered checks need the tested commit to be available on GitHub. Other checkouts validate source paths locally and defer GitHub-specific source fragments, whose behavior is not fully reproduced by local files. This limitation does not weaken anchor checks within the generated documentation.
For unpublished local commits, the documented fork-identity setting selects local source validation. Set it before the checking build and the rendered hook so generated example links use the same policy.
Rendered source links use the checked-out commit, so their URLs usually differ from those in the nightly cache and require a small number of fresh GitHub requests. Authored source links are checked locally, reducing requests there; established external links continue to benefit from the shared cache.
These rules model the proposed source tree. Maintenance branches need a deliberate policy when their links intentionally refer to a different tree.
Implementation details and suggested review order
GITHUB_REPOSITORYin CI, or an explicitlychee.repositoryGit setting followed by the conventionalupstream/originremotes locally.mainfor local source validation, when generating example URLs. Explicit checking-build overrides and release-tag behavior remain intact.The wrapper supplies remapping rules to native Lychee without rewriting source files or generated HTML. Public rendered rules preserve paths, queries, and fragments. Local rendered rules omit fragments only for the matched repository source links.
The helper participates in the cache fingerprint, so caches produced under a different mapping policy are not reused. The three-day lifetime, separate authored/rendered caches, and cache publication policy remain unchanged.
Validation
Local validation of this revision:
pre-commit run --all-filespasses, including the configured authored Lychee hook and workflow linting.The current full CI run checks
dd3100c23a6991756949142b7dc88c72ff8e84a9. Its first attempt passed 102 jobs, including both formerly queued Windows L4 jobs and the documentation build. Rendered Lychee passed on its first attempt with a cold cache: 13,524 unique links, six remaps, and no errors or timeouts. An unchanged CUDA graph timing test failed after 30 seconds, matching its callback timeout; a targeted retry of that job and the aggregate status check was requested without code changes.Checklist