fix(ci): stop the link check reporting the site against itself, and gate the heavy workflows - #326
Open
YuanYuYuan wants to merge 7 commits into
Open
fix(ci): stop the link check reporting the site against itself, and gate the heavy workflows#326YuanYuYuan wants to merge 7 commits into
YuanYuYuan wants to merge 7 commits into
Conversation
MkDocs writes site_url into site/sitemap.xml and into the rel=canonical tag of every page, so each run asks GitHub Pages about several hundred of its own URLs. That check queries the previous deployment, so a page added in the current build always 404s, and the request burst gets rate-limited. Fixes #323: a 503 reported on a page that is live and returns 200.
ci.yml, test.yml and rmw-zenoh-rs.yml selected work with paths-ignore. A deny-list has to name every file that must not build, so every file it does not name runs the full ROS matrix. .lychee.toml was one: a one-line comment in it started 4 interop distros, 4 Python distros, ROS tests, WASM, SHM, coverage and both macOS jobs. test.yml and rmw-zenoh-rs.yml also still ignored book/**, a directory the MkDocs migration removed, so every docs/ edit ran both. Each of the three now lists what it actually builds from. Two jobs had a wider interest than the build inputs, so they move out rather than widen the list back: - Check Formatting -> lint.yml, no path filter. The pre-commit check covers .rs, .toml, .yaml, .md, .py and .nix across the tree, so a config-only change must still reach it. It is also the cheapest job in CI. - hu-docs-repro -> hu-docs.yml, which adds docs/tools/**. It replays every documented hu command, and under paths-ignore a docs/** edit could change those commands without ever running it. docs.yml gains .lychee.toml, crates/*/examples/** and check-example-coverage.nu, which its own steps read.
The weekly check opened a new issue on every failing run, each with the identical title. Closing one left the others open saying the same thing. The run now looks for an open issue with that title first. It updates that one when links fail, opens a new one only when none is open, and closes the open one with a comment when every link resolves.
YuanYuYuan
changed the title
.config/nextest.toml sets slow-timeout, test-threads and fail-fast for every nextest leg in ci.yml and test.yml, and .cargo/config.toml carries the alias the WASM plugin build uses. Neither was listed, so a change to either ran no test that it changes. Also record why a required status check must not be added on top of these filters: a workflow the filter skips reports nothing, and a required check that never reports blocks the pull request. main requires no checks today.
An allow-list only skips what it does not name, so work landing somewhere new is silently never built. That is the one cost the allow-list trades for, and it is invisible: a green run and a skipped run look the same. lint.yml has no path filter, so it sees every change. It now diffs the pull request against its merge base and requires each file to match some workflow's paths, or to appear in the script's EXEMPT list with a reason. Both outcomes are fine; silence is not. The parse is strict in two ways, each covering a way this could rot quietly. A workflow with a paths: key that yields no patterns is an error, not an empty filter that silently covers nothing. And because Actions rejects YAML anchors, push and pull_request each carry their own copy of the list -- the script requires the copies to stay identical. Running it over the tree found six files that reach nothing: .clang-format, .vale.ini and four .vale/styles files. Both tools ship in the devshell and no job runs either, so they are exempt with that stated as the reason.
The coverage guard reported .clang-format and .vale.ini as files no job reads. Both are now read, by the hook set that lint.yml already runs on every change -- so they also run locally on commit rather than only in CI. vale could not run at all. .vale.ini named Vocab = ros-z, a name the rename to hiroz left behind, and the vocabulary sat in the Vale 2 location (<styles>/Vocabularies/) that Vale 3 replaced with <styles>/config/ vocabularies/. Either alone makes vale exit 2 before it lints anything. With both fixed it reported 412 alerts. 333 were spelling, over 50 distinct words, and nearly all were terms this project uses -- hiroz, hu, hiroz_msgs and hiroz_py among them, never added when ros-z was renamed. 50 terms go into the vocabulary and clear all 333. The other 79 were write-good Passive, Weasel and TooWordy. Passive voice is frequently the correct choice in reference prose, so those three rules are a style opinion rather than a defect; they are removed, and .vale.ini records why. Spelling is the rule with an objective answer. 'iroz' and 'nion' were not words: docs/tools/why-hu.md wrote '**H**iroz **U**nion', and mid-word bold splits both words in two. Adding those fragments to the vocabulary would mask real typos, so the line reads '**Hiroz Union**' instead. clang-format found 688 violations. 464 are in crates/hiroz-go/hiroz/hiroz_ffi.h, which cbindgen generates from crates/hiroz/build.rs -- formatting it would be undone by the next build, so the hook excludes it. The remaining 224 are in five hand-written files and this reformats them.
|
There was a problem hiding this comment.
Pull request overview
Refines CI triggering, link-report handling, and repository-wide lint enforcement.
Changes:
- Replaces heavy-workflow deny-lists with guarded allow-lists.
- Reuses and closes recurring broken-link issues.
- Adds Vale/clang-format hooks and applies formatting fixes.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
.lychee.toml |
Excludes generated self-links. |
.vale.ini |
Enables project spelling checks. |
.vale/styles/config/vocabularies/hiroz/accept.txt |
Adds project vocabulary. |
.vale/styles/write-good/Passive.yml |
Removes passive-voice rule. |
.vale/styles/write-good/TooWordy.yml |
Removes wordiness rule. |
.vale/styles/write-good/Weasel.yml |
Removes weasel-word rule. |
.github/workflows/ci.yml |
Adds allow-list and moves jobs. |
.github/workflows/docs-links.yml |
Reuses recurring report issues. |
.github/workflows/docs.yml |
Expands documentation triggers. |
.github/workflows/hu-docs.yml |
Extracts documentation reproduction job. |
.github/workflows/lint.yml |
Adds lint and path-coverage jobs. |
.github/workflows/rmw-zenoh-rs.yml |
Adds workflow allow-list. |
.github/workflows/test.yml |
Adds workflow allow-list. |
scripts/check-ci-path-coverage.py |
Guards workflow path coverage. |
nix/pre-commit.nix |
Adds Vale and clang-format hooks. |
docs/tools/why-hu.md |
Fixes emphasized product name. |
crates/hiroz-go/hiroz/callback_bridge.c |
Applies C formatting. |
crates/rmw-zenoh-rs/binding.hpp |
Reorders includes. |
crates/rmw-zenoh-rs/include/rmw_bridge.h |
Applies C++ formatting. |
crates/rmw-zenoh-rs/include/serde_bridge.h |
Applies C++ formatting. |
crates/rmw-zenoh-rs/src/serde_bridge.cc |
Applies C++ formatting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
scripts/test-release-version-semantics.sh reads .github/workflows/ release.yml and asserts against it. release.yml itself runs only on a v* tag, so no pull request exercises it and that script is its one check before a tag is pushed. Under the old deny-list an edit to release.yml ran ci.yml, which owned that script. Moving the job to hu-docs.yml dropped it: neither event listed release.yml, so the check silently stopped running. hu-docs.yml now lists it. The coverage guard should have caught this and did not. Its EXEMPT list carried a blanket .github/**, which swallowed every file under that directory -- including the one whose only check had just disappeared. The blanket is gone; a workflow that no path filter reaches now needs its own line and its own reason. Removing it immediately found a second instance: mkdocs-preview.yml has a paths: filter that omitted its own filename, so editing that workflow triggered nothing and it could not test itself. It now lists itself, as every other filtered workflow does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three CI defects, all visible on this PR's own first run: the weekly link check reports the site's own URL as broken, it opens a new issue every time it does, and a one-line edit to its config started the full ROS matrix.
Key Changes
fix(ci).lychee.tomlexcludeszettascalelabs.github.io/hirozcici.yml,test.yml,rmw-zenoh-rs.ymltrigger from an allow-list;Check Formattingandhu-docs-repromove to their own workflowsci(docs-links)ci.config/nextest.tomland.cargo/config.toml; record why a required check must not sit on top of a path filtercici1. The link check asked GitHub Pages about its own pages
mkdocs.ymlsetssite_url: https://zettascalelabs.github.io/hiroz/. MkDocs writes that URL intosite/sitemap.xmland into therel="canonical"tag of every page, and the check runs over the freshly builtsite/. Every run therefore asked GitHub Pages about several hundred of its own URLs.docs/site/sitemap.xmlentriesrel="canonical"of every pageGrepping
docs/andmkdocs.ymlfinds that URL only on thesite_urlline, so every occurrence in the built site is machine-generated. The exclusion removes no hand-written link from coverage.2.
paths-ignoreis a deny-list, so it ran everything it did not nameA deny-list has to name every file that must not build. Anything it misses runs the full matrix.
.lychee.tomlwas one such file — this PR's first commit started 4 interop distros, 4 Python distros, ROS tests, WASM, SHM, coverage and both macOS jobs, for a link-checker comment.test.ymlandrmw-zenoh-rs.ymlalso still ignoredbook/**, a directory the MkDocs migration removed, so everydocs/edit ran both of them.Each of the three now lists what it actually builds from. Two jobs had a wider interest than the build inputs, so they move out rather than widen the list back:
Check Formattinglint.yml, no path filter.rs,.toml,.yaml,.md,.pyand.nixacross the tree, so a config-only change must still reach it. It is also the cheapest job in CI (~1 min)hu-docs-reprohu-docs.yml, which addsdocs/tools/**hucommand. Underpaths-ignoreadocs/edit could change those commands without ever running itdocs.ymlgains.lychee.toml,crates/*/examples/**andcheck-example-coverage.nu— its own steps read all three, and it was not triggered by a change to any of them.Resulting triggers, simulated over the GitHub path-filter semantics (
*stops at/,**does not, a later!overrides an earlier match):.lychee.toml.github/workflows/docs-links.ymlcrates/hiroz/src/lib.rscrates/hiroz/README.mddocs/tools/hu.mddocs/index.mdCargo.toml.config/nextest.toml.cargo/config.tomlscripts/test-go.nucodecov.ymlmkdocs.ymlWriting the allow-list surfaced two files that the deny-list had also missed, in the opposite direction.
.config/nextest.tomlsetsslow-timeout,test-threadsandfail-fastfor every nextest leg inci.ymlandtest.yml;.cargo/config.tomlcarries the alias the WASM plugin build uses. Neither ran a single test it changes. Both are now listed.This shape is what GitHub documents, with one condition attached
These three workflows both include and exclude (
crates/**then!crates/**/*.md), whichpaths-ignorecannot express — the two filters cannot be combined for one event. The negation also follows the documented last-match-wins rule, and the docs require at least one positive pattern alongside any!, which every list here has.Important
The condition is branch protection. A workflow a path filter skips reports nothing, and a required check that never reports leaves the PR pending forever — the single most-reported problem with path filters (community#54877). Gating with a job-level
if:avoids it, because a job skipped that way reports success.mainrequires no checks today —required_status_checks.contextsand.checksare both empty — so nothing here can wedge a PR. Each filtered workflow now carries that condition as a comment, so whoever adds a required check reads it at the point they would break it.The allow-list's own cost, and the guard that removes it
An allow-list has to be extended when work lands somewhere new, and a missed entry is a silent skip rather than a wasted run — a green board and an empty board look identical. The layout bounds the exposure (401 of 431 tracked source files are under
crates/, 29 underscripts/, 1 undernix/, all allow-listed prefixes), but bounding is not closing.lint.ymlhas no path filter, so it sees every change.scripts/check-ci-path-coverage.pynow diffs the PR against its merge base and requires each changed file either to match some workflow'spaths, or to appear in anEXEMPTlist with a reason. Both outcomes are fine; silence is not.The parse is strict in two ways, each closing a way the guard itself could rot quietly:
paths:key but no pattern parsespushandpull_requestlists differRunning it over the whole tree found six files that reach nothing:
.clang-format,.vale.iniand four.vale/styles/files. Both tools shipped in the devshell and no job ran either. Rather than exempt them, the last commit wires them in — see below.Controls, all four measured:
rc=0, "every one triggers a workflow or is exempt"bindings/rc=1, names the fileci.yml'spathslist emptiedrc=1, "has a paths: key but no patterns parsed"ci.yml'spull_requestcopy onlyrc=1, "its paths: lists disagree between events"It is written in Python rather than Nushell like the rest of
scripts/because it runs on every pull request andpython3is already on the runner; installing nushell would cost more than the check.3. One report issue, not one per run
Every failing run opened another issue with the identical title, and closing one left the rest open saying the same thing. The run now looks for an open issue with that title first: it updates that one when links fail, opens a new one only when none is open, and closes the open one with a comment when every link resolves.
Wiring the two tools the guard found
They join the pre-commit hook set that
lint.ymlalready runs on every change, so they also run locally on commit rather than only in CI.vale could not run at all
.vale.ininamedVocab = ros-z— a name the rename to hiroz left behind — and the vocabulary sat in<styles>/Vocabularies/, the Vale 2 location that Vale 3 replaced with<styles>/config/vocabularies/. Either alone makes vale exit 2 before linting anything, which is why it was never wired.With both fixed it reported 412 alerts:
Vale.Spellinghiroz,hu,hiroz_msgs,hiroz_pyamong them, never added when ros-z was renamed. 50 terms go into the vocabulary and clear all 333write-good.Passivewrite-good.Weaselwrite-good.TooWordy.vale.inirecords why only spelling is enforced: it is the rule with an objective answer.Two of the 50 "words" were not words.
docs/tools/why-hu.mdwrote**H**iroz **U**nion, and mid-word bold splits both words in two, so vale sawirozandnion. Adding those fragments to the vocabulary would mask real typos, so the line reads**Hiroz Union**instead.clang-format
688 violations, of which 464 are in
crates/hiroz-go/hiroz/hiroz_ffi.h— generated by cbindgen fromcrates/hiroz/build.rs. Formatting it would be undone by the nextcargo build --features ffiand show up as a spurious diff, so the hook excludes it and the exclusion carries that reason. The remaining 224 are in five hand-written files, reformatted here:Note
Those headers are consumed by bindgen in
rmw-zenoh-rs'sbuild.rs, which needs ROS headers and cannot be built outside CI. Reformatting changes no token, but the claim that it builds rests on this PR'srmw_zenoh_rs Testsjob, not on a local run.What fails without this
#323:[503] https://zettascalelabs.github.io/hiroz/experimental/shm/ (at site/sitemap.xml:64:15). That page returns200on request, so the report is a false positive from rate limiting..lychee.toml:gh pr checkslisted 27 checks including all four interop distros, all four Python distros, ROS tests, WASM, SHM and coverage.Verification
curl -o /dev/null -w '%{http_code}'→200.lychee.tomlwithtomllib, matched the#323URL and the site root;docs.ros.organdgithub.com/ZettaScaleLabs/hiroz/...stay checked, and a lookalike with the dots replaced does not matchpathslistsyaml.safe_loadon all 12 files, thenpre-commit run yamllint→Passedgit ls-filesover.rs/.nu/.sh/.py/.go/.toml/.nix: 401 undercrates/, 29 underscripts/, 1 undernix/, the rest root-level configs — which is how.config/nextest.tomland.cargo/config.tomlwere foundpre-commit run --all-files→rc=0, all hooks pass. Then a deliberate typo indocs/index.md→ valerc=1namingTpyo,teh,wrod; a deliberately misformatted function inserde_bridge.cc→ clang-formatrc=1git statusoncrates/hiroz-go/hiroz/hiroz_ffi.hafter a full hook run → unchangedThe negative case cannot be observed on this PR: the allow-list includes
.github/workflows/ci.yml, which this PR edits, so the heavy matrix correctly runs here. Only a later config-only change shows the skip in production.Breaking Changes
None.