diff --git a/.github/workflows/docs-link-check.yml b/.github/workflows/docs-link-check.yml index 5213f78714..2e9441392a 100644 --- a/.github/workflows/docs-link-check.yml +++ b/.github/workflows/docs-link-check.yml @@ -6,6 +6,13 @@ name: Docs Link Check # toute la lane). Ce fichier garde `workflow_dispatch` pour le re-run manuel # sur `main` apres une panne CI (#9858). Les `paths:` d'origine vivent dans # `scripts/ci/fast_lane_registry.py` (TRANCHE1). +# +# Les deux voies n'excusent PAS les liens casses de la meme facon (#15766) : +# la garde de PR compare a sa base (`--base {base_ref}`), ou un lien deja +# casse n'est pas la regression de la branche ; ce dispatch, qui n'a pas de +# base a comparer, s'en remet seul au fichier de reference fige. C'est +# volontaire : `--baseline` n'est appele par aucune automatisation, donc ce +# fichier ne doit rien excuser de plus que ce qu'il contient deja. on: workflow_dispatch: diff --git a/scripts/check_docs_links.py b/scripts/check_docs_links.py index 53d06d224c..92d9800649 100644 --- a/scripts/check_docs_links.py +++ b/scripts/check_docs_links.py @@ -8,21 +8,31 @@ Also detects orphan .md files in docs/ (not referenced by any scanned file). Usage: - python scripts/check_docs_links.py # Full scan - python scripts/check_docs_links.py --baseline # Write baseline report - python scripts/check_docs_links.py --check # Check against baseline - python scripts/check_docs_links.py --quiet # Minimal output (for CI) - python scripts/check_docs_links.py --orphans # Also report orphan docs + python scripts/check_docs_links.py # Full scan + python scripts/check_docs_links.py --baseline # Write baseline report + python scripts/check_docs_links.py --check # Check against baseline + python scripts/check_docs_links.py --check --base origin/main + python scripts/check_docs_links.py --quiet # Minimal output (for CI) + python scripts/check_docs_links.py --orphans # Also report orphan docs + +``--check`` accepts two independent excuses for a broken link. The frozen +``baseline_docs_links.json`` covers paths with no comparison revision (manual +dispatch, plain full scan). ``--base REF`` additionally excuses a link that was +ALREADY broken at REF: without it, a single broken link on ``main`` reddens +``check-links`` on every open PR until someone regenerates the baseline, which +nothing does (#15766). Exit codes: - 0 = all links valid (or only pre-existing broken links in baseline mode) + 0 = all links valid (or only excused broken links) 1 = new broken links found (regression) 2 = error during execution """ import argparse import json +import posixpath import re +import subprocess import sys from dataclasses import dataclass, field from pathlib import Path @@ -170,17 +180,16 @@ def find_scan_files() -> list[Path]: return files -def scan_file(filepath: Path) -> list[LinkRef]: - """Extract all relative links from a markdown file. +def scan_content(content: str, rel_source: str) -> list[LinkRef]: + """Extract all relative links from markdown ``content``. + + ``rel_source`` is the repo-relative posix path the content comes from: link + targets resolve against its directory, so the same routine serves both the + working tree and a ``git show`` read of an arbitrary revision. Skips links inside fenced code blocks (```...```). """ refs = [] - try: - content = filepath.read_text(encoding="utf-8") - except (UnicodeDecodeError, PermissionError, OSError): - return refs - in_code_block = False for line_num, line in enumerate(content.splitlines(), 1): stripped = line.strip() @@ -199,10 +208,6 @@ def scan_file(filepath: Path) -> list[LinkRef]: text = match.group(1) target = match.group(2) if _is_valid_target(target): - try: - rel_source = str(filepath.relative_to(REPO_ROOT)).replace("\\", "/") - except ValueError: - rel_source = str(filepath) refs.append(LinkRef( source=rel_source, target=target, @@ -212,6 +217,19 @@ def scan_file(filepath: Path) -> list[LinkRef]: return refs +def scan_file(filepath: Path) -> list[LinkRef]: + """Extract all relative links from a markdown file on disk.""" + try: + content = filepath.read_text(encoding="utf-8") + except (UnicodeDecodeError, PermissionError, OSError): + return [] + try: + rel_source = filepath.relative_to(REPO_ROOT).as_posix() + except ValueError: + rel_source = str(filepath) + return scan_content(content, rel_source) + + def _is_quarto_render_target(html_path: Path, root: Path) -> bool: """Return whether Quarto generates ``html_path`` from a listed notebook.""" notebook = html_path.with_suffix(".ipynb") @@ -263,6 +281,113 @@ def check_link(target: str, source_path: Path, root: Path = REPO_ROOT) -> bool: ) +def _git_show(rev: str, rel_path: str) -> str | None: + """Content of ``rel_path`` at ``rev``, or None when it is not readable. + + The return code is inspected BEFORE the stdout is used: a partial clone can + exit non-zero and still emit a body, which would then be scanned as if it + were the file (#15387). + """ + proc = subprocess.run( + ["git", "show", f"{rev}:{rel_path}"], + cwd=REPO_ROOT, capture_output=True, text=True, + encoding="utf-8", errors="replace", + ) + return proc.stdout if proc.returncode == 0 else None + + +def _git_tree_files(rev: str) -> set[str] | None: + """Every blob path (repo-relative posix) at ``rev``, or None if unreachable. + + None is the caller's signal that the comparison revision could not be read; + the caller then falls back to the frozen baseline rather than inventing + regressions it cannot substantiate. + """ + proc = subprocess.run( + ["git", "ls-tree", "-r", "--name-only", rev], + cwd=REPO_ROOT, capture_output=True, text=True, + encoding="utf-8", errors="replace", + ) + if proc.returncode != 0: + return None + return {line.strip() for line in proc.stdout.splitlines() if line.strip()} + + +def _tree_dirs(files: set[str]) -> set[str]: + """Directories implied by a file listing (git stores no empty directory).""" + dirs: set[str] = set() + for name in files: + parts = name.split("/") + for i in range(1, len(parts)): + dirs.add("/".join(parts[:i])) + return dirs + + +def _link_exists_in_tree(target: str, source_rel: str, files: set[str], + dirs: set[str], quarto_yml: str | None) -> bool: + """Existence oracle over a git tree listing, mirroring ``check_link``.""" + try: + decoded = unquote(target) + except (ValueError, TypeError): + return False + + rel = posixpath.normpath(posixpath.join(posixpath.dirname(source_rel), decoded)) + if rel.startswith(("/", "..")): + return False # escapes the repo, as in check_link + if any(rel == sm or rel.startswith(sm + "/") for sm in SUBMODULE_PATHS): + return True + if rel in files or rel.rstrip("/") in dirs: + return True + # Quarto-generated pages are absent from the source tree by design: valid + # only when the sibling notebook exists and project.render lists it. + if rel.endswith(".html") and quarto_yml is not None: + notebook = rel[:-5] + ".ipynb" + return notebook in files and f'"{notebook}"' in quarto_yml + return False + + +def preexisting_broken(broken_refs: list[LinkRef], rev: str) -> set[tuple[str, str]] | None: + """Among ``broken_refs``, those already broken at ``rev``. + + Only the sources carrying a broken link at HEAD are read at ``rev``: the + question is never "what was broken there" but "was THIS link already + broken", so the base revision is consulted source by source instead of + being scanned wholesale. + + A link counts as pre-existing only when the source existed at ``rev``, + carried the same target, and that target was already absent there. A target + deleted by the branch, or a link the branch introduced, stays a regression. + + Returns None when ``rev`` cannot be read. + """ + if not broken_refs: + return set() + tree = _git_tree_files(rev) + if tree is None: + return None + dirs = _tree_dirs(tree) + quarto_yml = _git_show(rev, "_quarto.yml") + + wanted: dict[str, set[str]] = {} + for ref in broken_refs: + wanted.setdefault(ref.source, set()).add(ref.target) + + preexisting: set[tuple[str, str]] = set() + for source, targets in wanted.items(): + if source not in tree: + continue # created by this branch: nothing pre-existed + content = _git_show(rev, source) + if content is None: + continue + base_targets = {r.target for r in scan_content(content, source)} + for target in targets: + if target in base_targets and not _link_exists_in_tree( + target, source, tree, dirs, quarto_yml + ): + preexisting.add((source, target)) + return preexisting + + def find_orphan_docs(scanned_files: list[Path], all_refs: list[LinkRef]) -> list[str]: """Find .md files in docs/ not referenced by any link in scanned files.""" # Collect all link targets (resolved to repo-relative paths) @@ -344,17 +469,22 @@ def load_baseline(path: Path = BASELINE_PATH) -> dict | None: return None -def check_regression(result: ScanResult, baseline: dict) -> list[LinkRef]: - """Find broken links that are NOT in the baseline (new regressions).""" +def check_regression(result: ScanResult, baseline: dict, + preexisting: set[tuple[str, str]] | None = None) -> list[LinkRef]: + """Find broken links that are NOT already excused (new regressions). + + Two independent excuses apply. The frozen ``baseline`` covers the + non-PR paths (dispatch, plain full scan) where no comparison revision is + available. ``preexisting`` — computed against the branch's base revision — + covers the PR path: a link already broken before the branch is not this + branch's regression, and would otherwise redden every open PR at once + (#15766). + """ baseline_targets = { (b["source"], b["target"]) for b in baseline.get("broken_links", []) } - new_broken = [] - for ref in result.broken: - key = (ref.source, ref.target) - if key not in baseline_targets: - new_broken.append(ref) - return new_broken + excused = baseline_targets | (preexisting or set()) + return [ref for ref in result.broken if (ref.source, ref.target) not in excused] def format_report(result: ScanResult, show_orphans: bool = False) -> str: @@ -386,6 +516,9 @@ def main(): help="Write current state as baseline") parser.add_argument("--check", action="store_true", help="Check for regressions against baseline") + parser.add_argument("--base", metavar="REF", default=None, + help="With --check: a broken link already broken at REF " + "(e.g. origin/main) is not a regression") parser.add_argument("--orphans", action="store_true", help="Also report orphan docs") parser.add_argument("--quiet", action="store_true", @@ -403,30 +536,38 @@ def main(): return if args.check: + preexisting = None + if args.base: + preexisting = preexisting_broken(result.broken, args.base) + if preexisting is None and not args.quiet: + print(f"WARNING: {args.base!r} is not readable, comparing against the " + f"frozen baseline only.", file=sys.stderr) + baseline = load_baseline() - if baseline is None: + if baseline is None and preexisting is None: if not args.quiet: print("No baseline found. Run with --baseline first.") - # If no baseline, any broken link is a regression + # Nothing can excuse a broken link: every one is a regression if result.broken: if not args.quiet: print(format_report(result)) sys.exit(1) sys.exit(0) - new_broken = check_regression(result, baseline) + new_broken = check_regression(result, baseline or {}, preexisting) if new_broken: if not args.quiet: print(f"REGRESSION: {len(new_broken)} new broken link(s):") for ref in new_broken: print(f" {ref.source}:{ref.line} -> {ref.target}") - print(f"\nBaseline had {len(baseline.get('broken_links', []))} known broken.") + print(f"\nExcused: {len((baseline or {}).get('broken_links', []))} from baseline" + f" + {len(preexisting or ())} already broken at {args.base or '(no base)'}.") sys.exit(1) - else: - if not args.quiet: - print(f"OK: No new broken links. " - f"({len(result.broken)} pre-existing, {result.total_links} total)") - sys.exit(0) + + if not args.quiet: + print(f"OK: No new broken links. " + f"({len(result.broken)} pre-existing, {result.total_links} total)") + sys.exit(0) # Default: full scan if not args.quiet: diff --git a/scripts/ci/fast_lane_registry.py b/scripts/ci/fast_lane_registry.py index 7268945c35..b217152d07 100644 --- a/scripts/ci/fast_lane_registry.py +++ b/scripts/ci/fast_lane_registry.py @@ -417,9 +417,17 @@ class Guard: # couvre deja tout ce que ces trois gardes demandent. # --------------------------------------------------------------------------- TRANCHE1: list[Guard] = [ - # Forme 1 : scan global simple, sans base. Source : docs-link-check.yml - # (job `check-links`). Le nom du garde est le nom du JOB, pas celui du - # workflow -- c'est lui que le rollup affichait. + # Forme 1 : scan global. Source : docs-link-check.yml (job `check-links`). + # Le nom du garde est le nom du JOB, pas celui du workflow -- c'est lui que + # le rollup affichait. + # + # `--base {base_ref}` (#15766) : le baseline fige est a `broken_links: []` + # et rien ne le regenere, donc un seul lien casse sur `main` rendait ce + # garde rouge sur TOUTE PR ouverte (`check-links` + `Always-on guards` + + # `PR gate`), quel que soit le contenu de la PR. Comparer a la base rend + # l'excuse exacte : un lien deja casse avant la branche n'est pas la + # regression de la branche. Le baseline fige reste la voie des executions + # hors PR (dispatch, scan complet), ou aucune base n'est disponible. Guard( name="check-links", source="docs-link-check.yml", @@ -428,9 +436,11 @@ class Guard: ".claude/rules/**", "docs/**", "**/README.md", "scripts/check_docs_links.py", ], - argv=["python", "scripts/check_docs_links.py", "--check"], + argv=["python", "scripts/check_docs_links.py", "--check", + "--base", "{base_ref}"], blocking=True, absorbed=True, + needs_base=True, ), # Forme 2 : scan globs, bloque sur la convention zero-pad des series # DECLAREES (#11840/#12586, portee explicite #15489 defaut 5). Source : diff --git a/scripts/tests/test_check_docs_links.py b/scripts/tests/test_check_docs_links.py index fc33535c12..531158a621 100644 --- a/scripts/tests/test_check_docs_links.py +++ b/scripts/tests/test_check_docs_links.py @@ -9,6 +9,7 @@ """ import json +import subprocess import textwrap from pathlib import Path @@ -18,6 +19,7 @@ import sys sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +import check_docs_links from check_docs_links import ( BASELINE_PATH, LinkRef, @@ -28,11 +30,15 @@ find_scan_files, format_report, load_baseline, + preexisting_broken, run_scan, + scan_content, scan_file, write_baseline, _is_valid_target, + _link_exists_in_tree, _should_skip, + _tree_dirs, REPO_ROOT, ) @@ -407,6 +413,149 @@ def test_fixed_link_not_flagged(self): assert len(new_broken) == 0 +class TestPreexistingExcuse: + """--check accepts links already broken at the comparison revision (#15766).""" + + def test_preexisting_link_is_not_a_regression(self): + result = ScanResult(broken=[ + LinkRef(source="README.md", target="docs/gone.md", line=3, text="gone"), + ]) + new_broken = check_regression( + result, {}, preexisting={("README.md", "docs/gone.md")}) + assert new_broken == [] + + def test_unrelated_preexisting_does_not_mask_a_new_link(self): + result = ScanResult(broken=[ + LinkRef(source="README.md", target="docs/gone.md", line=3, text="gone"), + LinkRef(source="CLAUDE.md", target="docs/fresh.md", line=9, text="fresh"), + ]) + new_broken = check_regression( + result, {}, preexisting={("README.md", "docs/gone.md")}) + assert [r.target for r in new_broken] == ["docs/fresh.md"] + + def test_baseline_and_preexisting_both_excuse(self): + baseline = {"broken_links": [ + {"source": "README.md", "target": "docs/old.md", "line": 1, "text": "old"}, + ]} + result = ScanResult(broken=[ + LinkRef(source="README.md", target="docs/old.md", line=1, text="old"), + LinkRef(source="PARCOURS.md", target="docs/gone.md", line=2, text="gone"), + ]) + new_broken = check_regression( + result, baseline, preexisting={("PARCOURS.md", "docs/gone.md")}) + assert new_broken == [] + + def test_no_preexisting_keeps_legacy_semantics(self): + """Omitting the comparison revision behaves exactly as before.""" + result = ScanResult(broken=[ + LinkRef(source="README.md", target="docs/gone.md", line=3, text="gone"), + ]) + assert len(check_regression(result, {})) == 1 + + +class TestLinkExistsInTree: + """The git-tree existence oracle mirrors check_link on a file listing.""" + + FILES = {"docs/a.md", "docs/sub/b.md", "README.md"} + + def test_existing_file(self): + assert _link_exists_in_tree("./a.md", "docs/a.md", self.FILES, + _tree_dirs(self.FILES), None) + + def test_missing_file(self): + assert not _link_exists_in_tree("./nope.md", "docs/a.md", self.FILES, + _tree_dirs(self.FILES), None) + + def test_parent_traversal_inside_repo(self): + assert _link_exists_in_tree("../README.md", "docs/a.md", self.FILES, + _tree_dirs(self.FILES), None) + + def test_directory_target(self): + assert _link_exists_in_tree("./sub/", "docs/a.md", self.FILES, + _tree_dirs(self.FILES), None) + + def test_escaping_the_repo_is_broken(self): + assert not _link_exists_in_tree("../../outside.md", "docs/a.md", self.FILES, + _tree_dirs(self.FILES), None) + + def test_html_needs_listed_notebook(self): + files = self.FILES | {"docs/nb.ipynb"} + dirs = _tree_dirs(files) + assert not _link_exists_in_tree("./nb.html", "docs/a.md", files, dirs, "") + assert _link_exists_in_tree("./nb.html", "docs/a.md", files, dirs, + '"docs/nb.ipynb"') + + def test_submodule_path_is_valid(self, monkeypatch): + monkeypatch.setattr(check_docs_links, "SUBMODULE_PATHS", {"vendor/lib"}) + assert _link_exists_in_tree("vendor/lib/x.py", "README.md", self.FILES, + _tree_dirs(self.FILES), None) + + +class TestPreexistingBrokenAgainstRealGit: + """End-to-end: only links already broken at the base revision are excused.""" + + @staticmethod + def _git(root: Path, *args: str) -> None: + subprocess.run( + ["git", "-c", "user.email=t@example.com", "-c", "user.name=t", *args], + cwd=root, check=True, capture_output=True, + ) + + def _fixture(self, tmp_path: Path, monkeypatch) -> Path: + root = tmp_path / "repo" + (root / "docs").mkdir(parents=True) + self._git(root, "init", "-q") + # Base revision: one link whose target is already missing, one that works. + (root / "docs" / "present.md").write_text("# present\n", encoding="utf-8") + (root / "docs" / "a.md").write_text( + "[gone](./missing.md)\n[ok](./present.md)\n", encoding="utf-8") + self._git(root, "add", "-A") + self._git(root, "commit", "-q", "-m", "base") + # Head revision: unchanged a.md, plus a new file carrying a new broken link. + (root / "docs" / "b.md").write_text("[newgone](../nope.md)\n", encoding="utf-8") + self._git(root, "add", "-A") + self._git(root, "commit", "-q", "-m", "head") + monkeypatch.setattr(check_docs_links, "REPO_ROOT", root) + return root + + def test_only_already_broken_links_are_excused(self, tmp_path, monkeypatch): + self._fixture(tmp_path, monkeypatch) + refs = [ + LinkRef(source="docs/a.md", target="./missing.md", line=1, text="gone"), + LinkRef(source="docs/a.md", target="./present.md", line=2, text="ok"), + LinkRef(source="docs/b.md", target="../nope.md", line=1, text="newgone"), + ] + excused = preexisting_broken(refs, "HEAD~1") + + assert excused == {("docs/a.md", "./missing.md")} + # The link whose target existed at base is a real regression now, and the + # file added by this branch has nothing to excuse it. + remaining = check_regression(ScanResult(broken=refs), {}, excused) + assert sorted(r.target for r in remaining) == ["../nope.md", "./present.md"] + + def test_unreadable_revision_returns_none(self, tmp_path, monkeypatch): + self._fixture(tmp_path, monkeypatch) + refs = [LinkRef(source="docs/a.md", target="./missing.md", line=1, text="x")] + assert preexisting_broken(refs, "no-such-rev-15766") is None + + def test_no_broken_refs_short_circuits(self, tmp_path, monkeypatch): + """Nothing broken at HEAD means no revision needs to be read at all.""" + monkeypatch.setattr(check_docs_links, "REPO_ROOT", tmp_path) + assert preexisting_broken([], "no-such-rev-15766") == set() + + def test_scan_content_matches_scan_file(self, tmp_path, monkeypatch): + """The extracted scanner keeps the on-disk behaviour byte for byte.""" + f = tmp_path / "x.md" + body = "# t\n\n[ok](./y.md)\n\n```\n[skip](./z.md)\n```\n" + f.write_text(body, encoding="utf-8") + monkeypatch.setattr(check_docs_links, "REPO_ROOT", tmp_path) + from_file = scan_file(f) + from_content = scan_content(body, "x.md") + assert [(r.source, r.target, r.line) for r in from_file] == \ + [(r.source, r.target, r.line) for r in from_content] + assert [r.target for r in from_content] == ["./y.md"] + + class TestSelfCheck: """Acceptance: the script itself does not break anything."""