fix(hack): make lint-docs-links actually run on macOS - #6728
Merged
Conversation
The lint-docs-links pre-commit hook passed vacuously on every macOS
commit: its link extraction used grep -oP, BSD grep has no -P, and the
trailing `|| true` swallowed the "invalid option" failure, so the loop
saw zero links and the scope check succeeded over nothing. Measured on
this checkout: 0 links extracted by the old pipeline, 2798 by the fixed
one, across the 211 markdown files under docs/.
Two bugs, not one, and the second only detonates once the first is
fixed: with links actually flowing, `realpath -m` is next in the loop,
and BSD realpath has no -m either — under set -e the hook would go from
silently passing to hard-failing on every macOS commit that touches
docs/. So both are replaced together:
- Extraction is grep -oE '\]\([^)]+' piped through sed to strip the
literal "](". Same match as the PCRE lookbehind-\K original: all
occurrences per line, up to the next ")" or end of line, closing paren
not required. Verified equivalent across the whole docs corpus — 2798
links, zero mismatches against the original pattern's semantics.
- Resolution is python3 os.path.realpath, which resolves what exists
and normalizes the rest — GNU `realpath -m` semantics for targets
that may not exist. python3 is guaranteed wherever this hook runs,
because pre-commit itself runs on it.
Behavior on Linux CI is unchanged: same links, same resolution, and the
full docs/ corpus passes identically. On macOS the hook now does its
job — a synthetic file with two escaping links (../../../../etc/passwd
and ../..) is rejected with exactly those two reported, while anchors,
mailto, absolute URLs, and missing-but-in-repo targets pass through as
before.
Found while sweeping grep -P portability for the prior-SHA extractor
fix; this was the one site that is not Linux-only automation — the
pre-commit config wires it over ^docs/.*\.md$ on developer machines.
Verification: shellcheck clean; make lint exit 0; extraction parity,
full-corpus run, and the synthetic escape test all on darwin/arm64
(BSD grep 2.6.0-FreeBSD, /bin/realpath without -m).
Signed-off-by: guy oron <goron@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
5 tasks
PR Summary by QodoMake docs link linting portable on macOS
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Review on the previous commit flagged the cost correctly: spawning an interpreter per relative link put a full-corpus run at ~70s on this checkout (2798 links). Link candidates are now streamed to a single python3 process as NUL-terminated records — file_dir, rel_file, target, tab-separated, target last so a tab inside a link target cannot shift the fields, NUL-terminated so targets with spaces survive — and offenders come back newline-delimited, which is safe because targets are read line-by-line and cannot contain one. Same corpus now lints in under 3s. Resolution semantics are unchanged: os.path.realpath per target against the same root-prefix check, scheme/anchor filtering still in the shell before Python ever sees a link. Re-verified: full corpus passes identically, a synthetic file with three escaping links (one with a space in the target) reports exactly those three, a link-free file exits 0, shellcheck clean. Signed-off-by: guy oron <goron@redhat.com>
Contributor
Author
|
Fixed in 5f31fcf — good catch on the cost. Candidates now stream to a single python3 process (NUL-delimited records, target last so embedded tabs can't shift fields), offenders come back newline-delimited. Full corpus: ~70s → under 3s on this checkout, same verdicts — corpus passes, a synthetic file with three escaping links (one with a space in the target) reports exactly those three. |
rh-hemartin
approved these changes
Aug 31, 2026
Member
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:08 AM UTC · Completed 7:23 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.65 |
|
Risk Assessment: low (1/5) DetailsSingle-file 63-line change to a hack/ linting script with no protected paths, no security sensitivity, no CI or dependency changes, and a non-bot returning contributor. |
ReviewFindingsLow
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
fullsend-ai-review
Bot
added
the
ready-for-merge
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
2 participants
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.
Heyaaaa : )
Summary
Spun out of review on #6676 (@waynesun09's catch):
hack/lint-docs-linkspassed vacuously on every macOS commit — BSD grep has no-P, the|| trueswallowed it, so the loop saw zero links. Measured here: 0 links extracted by the old pipeline, 2798 by the fixed one, across 211 docs files.It turned out to be two bugs: once links actually flow,
realpath -mis next in the loop, and BSD realpath has no-meither — a grep-only fix would flip the hook from silently passing to hard-failing on every macOS docs commit. Both replaced together.Related Issue
None — raised in review on #6676.
Changes
grep -oE '\]\([^)]+'piped through sed to strip the](— same match as the PCRE original, all occurrences per line.python3 os.path.realpathinstead ofrealpath -m— same resolve-what-exists semantics, and python3 is guaranteed here since pre-commit itself runs on it.Testing
make lintpasses (stage changes first, then run)Extraction parity across all 211 docs files (2798 links, zero mismatches vs the original pattern); full corpus passes identically to Linux CI; a synthetic file with two escaping links is rejected with exactly those two. shellcheck clean, darwin/arm64.
Checklist
!for breaking changes)