chore(ci): gate the registry README sources against silent staleness - #1739
chore(ci): gate the registry README sources against silent staleness#1739kwit75 wants to merge 1 commit into
Conversation
Closes #1738. The four registry READMEs are not in git — each package's scripts/tasks.js copies one from docs/ at build time, which is why those directories have no README.md. The copy is silent when it breaks, and a registry README cannot be corrected without publishing a new version, so a wrong one stays live until the next release of that artifact. Adds .github/scripts/check-readme-sources.mjs, run as a small CI job. It fails on the three ways this breaks: 1. a README_SRC pointing at a file that does not exist 2. a README.md committed into a package directory, shadowing the generated one 3. a relative link or <img src> in a source README (3) is the one worth automating: each registry resolves relative paths against ITSELF rather than against GitHub, so a README that renders perfectly in the repo can render broken on npm, PyPI and the Marketplace at the same time. Currently all four sources are clean (11 links in the Python one, 15 in the VS Code one, zero relative) — the check is there to keep it that way rather than to fix anything. The script also fails if it finds NO tasks.js referencing README_SRC at all, rather than passing vacuously: an empty match would mean the copy convention had moved and the gate had silently stopped checking anything, which is the exact class of failure it exists to prevent. Verified in both directions, because a gate that only ever passes is decoration: clean tree -> exit 0 README_SRC renamed to a missing file -> exit 1, names the file README.md committed in a package -> exit 1, names the path relative link + relative <img> -> exit 1, names file:line and target Exit codes checked without a pipe — `$?` after `node ... | head` reports head's status, not node's, and reading it that way is how a non-gating gate gets shipped. Lives in .github/scripts/ rather than build/ because build/ is gitignored; the script would have existed locally and been missing in CI. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015nTVr6jfSFYm1GppxbjghP
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
📝 WalkthroughWalkthroughAdds a Node.js README source validator that checks README copy configuration, shadowing, and relative links or images. A dedicated GitHub Actions job runs the validator with Node.js v22. ChangesREADME source validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant CheckScript as check-readme-sources.mjs
participant Repository as tasks.js and docs files
GitHubActions->>CheckScript: Execute README source check
CheckScript->>Repository: Discover README_SRC tasks
CheckScript->>Repository: Validate source files and links
Repository-->>CheckScript: Return filesystem contents
CheckScript-->>GitHubActions: Report success or exit 1
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
.github/scripts/check-readme-sources.mjsOops! Something went wrong! :( ESLint: 9.39.5 TypeError: expand is not a function Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/check-readme-sources.mjs:
- Around line 103-131: Extend the link validation around the existing LINK and
<img src> checks to also inspect Markdown reference definitions such as “[name]:
./target” and HTML <a href="..."> attributes. Reuse ABSOLUTE to ignore absolute,
mailto, and fragment targets, and add the same relative-target diagnostics with
appropriate link context; include fixtures covering both reference-style and
HTML anchor links.
In @.github/workflows/ci.yml:
- Around line 226-236: Update the ci-ok job to include readme-sources in its
needs list and diagnostic result output, ensuring a failing README validator
causes CI OK to fail and remains visible in the status summary.
- Around line 231-234: Update the README sources job around actions/checkout and
actions/setup-node to set job-level permissions to contents: read and configure
actions/checkout with persist-credentials: false. Keep the existing checkout
revision and Node setup unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 370d558f-8942-4dcf-9b2f-7b81da20ea00
📒 Files selected for processing (2)
.github/scripts/check-readme-sources.mjs.github/workflows/ci.yml
| const LINK = /(!?)\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)/g; | ||
| const ABSOLUTE = /^(https?:\/\/|mailto:|#)/; | ||
|
|
||
| for (const docFile of [...sources].sort()) { | ||
| const text = readFileSync(docFile, 'utf8'); | ||
| const rel = relative(ROOT, docFile); | ||
| const lines = text.split('\n'); | ||
|
|
||
| lines.forEach((line, i) => { | ||
| for (const match of line.matchAll(LINK)) { | ||
| const [, bang, target] = match; | ||
| if (ABSOLUTE.test(target)) continue; | ||
| problems.push( | ||
| `${rel}:${i + 1}: ${bang ? 'image' : 'link'} target "${target}" is ` + | ||
| `relative — registries resolve it against themselves, not GitHub. ` + | ||
| `Use an absolute URL.` | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| // Bare <img src="..."> too — HTML is common in badge rows and PyPI sanitises | ||
| // it inconsistently, but a relative src is broken everywhere regardless. | ||
| for (const match of text.matchAll(/<img\s[^>]*src=["']([^"']+)["']/gi)) { | ||
| if (!ABSOLUTE.test(match[1])) { | ||
| problems.push( | ||
| `${rel}: <img src="${match[1]}"> is relative — use an absolute URL` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Catch reference-style and HTML anchor links.
Relative targets in reference definitions ([docs]: ./docs) and <a href="./docs"> bypass this gate entirely. That allows exactly the registry-broken links this check is intended to reject. Validate reference destinations and HTML href attributes in addition to inline Markdown and <img src>. Add fixtures for both forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/check-readme-sources.mjs around lines 103 - 131, Extend the
link validation around the existing LINK and <img src> checks to also inspect
Markdown reference definitions such as “[name]: ./target” and HTML <a
href="..."> attributes. Reuse ABSOLUTE to ignore absolute, mailto, and fragment
targets, and add the same relative-target diagnostics with appropriate link
context; include fixtures covering both reference-style and HTML anchor links.
| readme-sources: | ||
| name: README sources | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 | ||
| with: | ||
| node-version: 22 | ||
| # Dependency-free (node:fs + node:path only), so no install step. | ||
| - run: node .github/scripts/check-readme-sources.mjs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the README gate block CI OK.
ci-ok is documented as the only required branch-protection job, but its needs list and result output omit readme-sources. A failing validator therefore leaves CI OK green and does not enforce this gate. Add readme-sources to ci-ok.needs and its diagnostic output.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 231-231: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 232-232: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 226 - 236, Update the ci-ok job to
include readme-sources in its needs list and diagnostic result output, ensuring
a failing README validator causes CI OK to fail and remains visible in the
status summary.
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 | ||
| with: | ||
| node-version: 22 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow outline:"
wc -l .github/workflows/ci.yml
sed -n '1,280p' .github/workflows/ci.yml
echo
echo "Relevant checkout/setup-node/git usages:"
rg -n "actions/checkout|actions/setup-node|persist-credentials|permissions:|permissions: contents|git |credential|gh " .github/workflows/ci.yml
echo
echo "Git usages in tracked repository files, showing contexts:"
rg -n "git " .github/workflows/ci.yml . || trueRepository: rocketride-org/rocketride-server
Length of output: 27448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "README sources job with context:"
sed -n '216,310p' .github/workflows/ci.yml
echo
echo "All checkout steps with persist-credentials/job permissions around them:"
python3 - <<'PY'
from pathlib import Path
p=Path('.github/workflows/ci.yml')
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 'actions/checkout' in l:
start=max(1,i-4); end=min(len(lines),i+7)
print(f'\n# checkout around line {i}')
for n in range(start,end+1):
print(f'{n}: {lines[n-1]}')
PYRepository: rocketride-org/rocketride-server
Length of output: 9463
Scope this checkout to read-only credentials.
The README sources job reads the checkout only to run a local script; configure actions/checkout with persist-credentials: false and set the job-level permissions: contents: read so the runner-local token is not left writable in the Git config.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 231-231: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 232-232: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 231 - 234, Update the README sources
job around actions/checkout and actions/setup-node to set job-level permissions
to contents: read and configure actions/checkout with persist-credentials:
false. Keep the existing checkout revision and Node setup unchanged.
Source: Linters/SAST tools
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks for this, @kwit75 — the gate is genuinely useful and the write-up (the | head exit-code trap especially) is first-rate.
Requesting changes on the wiring, not the idea. The readme-sources job runs the script correctly — no continue-on-error, and it scans the whole tree so it is not diff-aware — but it is never added to ci-ok.needs, and ci-ok is documented as the only Required branch-protection check. A failing gate therefore turns the README sources job red while the required CI OK stays green, so the merge is still allowed and the gate does not gate. Proof: grep -n 'readme-sources' .github/workflows/ci.yml returns only the job (lines 226/236) and nothing in ci-ok.needs (line 319). Fix: add readme-sources to ci-ok.needs and to its diagnostic echo, then break a README_SRC and confirm CI OK (not just README sources) reports failure. Same point CodeRabbit raises at #1739 (comment) (Major).
Two more blockers. (1) The 149-line script has no committed regression test — the four cases in the description were checked by hand only; please add a test (fixtures plus expected exit codes) so the gate cannot silently regress. (2) CodeRabbit #1739 (comment) (Major) is valid — reference-style links ([x]: ./y) and <a href="./y"> bypass the LINK and <img> checks, which is exactly the relative-link class this gate exists to catch.
Non-blocking: CodeRabbit #1739 (comment) matches every other checkout in this workflow (top-level permissions: contents: read already applies, the job uploads no artifacts, and setup-node is given no cache: input so the cache-poisoning half is a false positive) — a repo-wide hardening call, not a blocker for this PR. One inline nit below.
| // Run: node .github/scripts/check-readme-sources.mjs | ||
| // ============================================================================= | ||
|
|
||
| import { readFileSync, existsSync, readdirSync, statSync } from 'node:fs'; |
There was a problem hiding this comment.
nit — statSync is imported but never used.
This line imports statSync from node:fs, but the script never calls it (only readFileSync, existsSync, and readdirSync are used). Drop it to keep the import clean.
| import { readFileSync, existsSync, readdirSync, statSync } from 'node:fs'; | |
| import { readFileSync, existsSync, readdirSync } from 'node:fs'; |
2 participants
Closes #1738. Follow-up to @charliegillet's #1736 — he asked how the five registry READMEs stay up to date, and the honest answer had two halves.
The half that was already done
The CI integration already exists. Each package copies its own README from
docs/at build time, which is why none of those directories has aREADME.md:Also worth stating because it sets the ceiling: no registry lets a README change without publishing a new version. npm serves it from the tarball, PyPI's description is per-release, the Marketplace and OpenVSX need a new extension version. So "up to date" can only mean "correct at each release", and the five pages legitimately diverge between releases because the artifacts ship on different cadences.
The half that was missing
Nothing verified the copy happened, or that what shipped matched the source. This adds
.github/scripts/check-readme-sources.mjsas a small CI job that fails on the three ways it breaks:README_SRCpoints at a missing fileREADME.mdcommitted into a package dir<img src>in a source README(3) is the one worth automating. A README with relative links renders perfectly in the repo and broken on npm, PyPI and the Marketplace simultaneously, and
vsceneeds--baseContentUrlto have any chance at all. All four sources are currently clean — 11 links in the Python one, 15 in the VS Code one, zero relative targets — so this keeps a good state rather than fixing a bad one.It also fails if it finds no
tasks.jsreferencingREADME_SRCat all, rather than passing vacuously. An empty match would mean the convention had moved and the gate had quietly stopped checking anything, which is precisely the failure it exists to prevent.Verified in both directions
A gate that only ever passes is decoration, so:
One note on how that was checked, because it is the trap: exit codes were read without a pipe.
node script.mjs | headreportshead's status in$?, not node's — my first run showedrc=0on all three failing cases and the script was fine all along. Reading it that way is how a non-gating gate gets merged.Two placement details
.github/scripts/, notbuild/—build/is gitignored, so the file would have existed on my machine and been missing in CI.git addrefusing it is what caught that.node:fs+node:pathonly), so the job is checkout + setup-node + onerun. No install step, ~10s.Not touching the existing copy steps or Charlie's content — this only adds the check.
Summary by CodeRabbit
New Features
Bug Fixes