diff --git a/Makefile b/Makefile index 272eac0f24..5a84e35938 100644 --- a/Makefile +++ b/Makefile @@ -44,6 +44,15 @@ serve_hugo: check_page_sizes: @python3 build/check_page_sizes.py public +# Report pages that moved without gaining an alias for their old URL, so the old +# URL now 404s. Reads git history, so it needs no build. Warn-only. +check_aliases: + @python3 build/check_missing_aliases.py --all + +# The same sweep, but writing the missing aliases into frontmatter. +check_aliases_fix: + @python3 build/check_missing_aliases.py --all --fix + clean: @rm -Rf ./public/ @rm -Rf ./resources/ diff --git a/build/check_missing_aliases.py b/build/check_missing_aliases.py new file mode 100644 index 0000000000..9558ea6bae --- /dev/null +++ b/build/check_missing_aliases.py @@ -0,0 +1,808 @@ +"""Report content pages that moved without gaining an alias for their old URL. + +Renaming a content file changes its published URL, and the old URL dies unless +the page declares an ``aliases:`` entry for it. That entry is author-declared +and therefore unreliable: measured over this repo's history, only 291 of 598 +URL-changing moves carry a matching alias, and it fails inconsistently even +within a single commit (``155277839`` moved LangCache and Agent Memory together; +LangCache got an alias, Agent Memory did not, and its old URL 404s today). + +This scans git history for renames, works out which ones actually changed a +published URL, and reports those with no alias. ``--fix`` writes the missing +aliases into frontmatter. + +Nine things make a naive version of this worse than useless -- a first attempt +reported 961 missing aliases against a true 253, nearly three quarters of it +noise, and every false positive looked plausible in a list -- so each of the +nine is handled explicitly: + +1. **Hugo bundles.** ``index.md`` (leaf) and ``_index.md`` (branch) both publish + at the containing directory's URL, so neither name appears in the URL and + renaming ``foo/index.md`` to ``foo.md`` changes nothing. 527 of this repo's + renames are of that kind -- the single largest source of false positives. +2. **Non-published directories.** ``content/embeds/`` carries a + ``build.render: never`` cascade, and the historical ``content/_embeds/`` + never reached the site either (240 renames between them). The rule is read + off the tree, not hardcoded -- and most of those files have no frontmatter, + so there would be nowhere to put an alias in any case. +3. **``url:`` frontmatter** overrides the path-derived URL. It is used on + exactly the versioned trees and nowhere else, so those are skipped. +4. **Chains.** A page moved twice must resolve to its final home. +5. **Path reuse.** An old URL may be occupied by a different page today, and + must never be redirected (22 cases). +6. **Declared-but-not-a-list aliases.** 88 files declare the key with no value + (49 spelled ``null``, 39 bare) and 104 give it a bare scalar rather than a + list, so a check that assumes a list silently under-reports. A scalar is + also whitespace-separated, because Hugo casts it with ``cast.ToStringSlice``: + one file folds a scalar across two lines and Hugo publishes both halves as + working aliases, where PyYAML reads the single string ``"/a/ /b/"``. Trusting + the YAML library over Hugo there cost a false positive against a page that + was never broken. +7. **Collisions.** 25 of the gaps name a URL another page already claims as its + own alias. Hugo resolves that by picking one arbitrarily and warning, so + adding the alias unattended would make the redirect ambiguous rather than + fix it. Those are reported for a human and never auto-fixed. +8. **Drafts.** 31 files are drafts, and production builds pass no + ``--buildDrafts``, so a draft publishes nothing at all -- *including its + aliases*. An alias added to one is a silent no-op, and counting a draft as + occupying a URL would suppress a real redirect. Caught by building the + corpus and finding two stubs Hugo declined to emit, which is the only reason + this is here rather than still latent. +9. **Pages split into a section.** ``X.md`` becoming ``X/.md`` turns the + old URL into a section URL while the file becomes one page inside it, so + redirecting the old landing page to that one child is usually wrong -- a + reader holding the old link wants the new landing page. git records lineage, + and lineage is not equivalence. 6 such splits exist here and 6 aliases reach + their target through one, so they are reported for a person rather than + guessed at. A split to ``X/_index.md`` is fine and not counted, because the + URL does not change. + +Warn-only by default (exit 0), like check_page_sizes; pass ``--fail`` to make CI +block on offenders. + +See DOC-6951. +""" + +# `X | None` annotations are 3.10+; local dev machines are still on 3.9. +from __future__ import annotations + +import argparse +import json +import logging +import os +import re +import subprocess +import sys +from dataclasses import dataclass, field + +logger = logging.getLogger("check_missing_aliases") + +# Exit codes. A finding and a failure must not share one: a caller that cannot +# tell them apart will report "some aliases could not be added" when what actually +# happened is that git fell over, which sends whoever reads it looking in the wrong +# place. Only meaningful with --fail; without it the scan is warn-only and always +# exits 0 unless it could not run at all. +EXIT_OK = 0 +EXIT_FINDINGS = 1 # gaps remain, or --fix could not place some aliases +EXIT_ERROR = 2 # the scan itself failed and reported nothing usable + +CONTENT = "content" + +# A path segment that looks like a semver version marks the versioned trees, +# which set `url:` in frontmatter and so cannot have their URL derived from +# their path. Matches two- and three-component versions (7.8, 0.10.0). +VERSIONED = re.compile(r"/[0-9]+\.[0-9]+(\.[0-9]+)?/") + +# git's default rename-detection similarity. Measured on this repo: 20% finds +# 617 URL-changing moves, 50% finds 598, 90% finds 477 -- so the default is +# close to the ceiling, and the tail that reads as delete-plus-add rather than a +# rename (a file renamed and heavily rewritten at once) is about 3.5%. +DEFAULT_THRESHOLD = 50 + +# Leading slash is effectively universal in this repo (918 of 929 entries). +# Trailing slash is a genuine 54/46 split with no house convention, so --fix +# picks one and stays consistent rather than guessing per file. +ALIAS_TEMPLATE = "/{url}/" + +# YAML spellings of "this key has no value". 49 files write `aliases: null` and +# 39 leave the key bare; both must be treated as empty, not as a one-item list +# containing the string "null". +NO_VALUE = ("", "null", "~") + + +@dataclass +class Move: + """A rename that changed a page's published URL.""" + + old_path: str + new_path: str + old_url: str + new_url: str + date: str + commit: str + aliased: bool = False + occupied: bool = False + target_draft: bool = False + split_at: str = "" + collides_with: list[str] = field(default_factory=list) + + @property + def actionable(self) -> bool: + """True when the alias can be added safely and without a judgment call.""" + return not (self.aliased or self.occupied or self.target_draft + or self.split_at or self.collides_with) + + +@dataclass +class FileFix: + """Aliases to add to one file.""" + + path: str + aliases: list[str] = field(default_factory=list) + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + scope = parser.add_mutually_exclusive_group() + scope.add_argument("--range", dest="rev_range", default="origin/main..HEAD", + help="revision range to scan (default: origin/main..HEAD)") + scope.add_argument("--all", action="store_true", + help="scan the whole history instead of a range") + parser.add_argument("--threshold", type=int, default=DEFAULT_THRESHOLD, + help=f"rename similarity %% (default: {DEFAULT_THRESHOLD})") + parser.add_argument("--fix", action="store_true", + help="write the missing aliases into frontmatter") + parser.add_argument("--json", dest="json_out", metavar="PATH", + help="also write the findings as JSON") + parser.add_argument("--github", action="store_true", + help="emit GitHub Actions warning annotations") + parser.add_argument("--fail", action="store_true", + help="exit 1 if any move is missing an alias (see EXIT_* below)") + return parser.parse_args() + + +def git(*args: str) -> str: + return subprocess.run(["git", *args], capture_output=True, text=True, + check=True).stdout + + +# --------------------------------------------------------------------------- # +# path -> URL +# --------------------------------------------------------------------------- # + +def to_url(path: str) -> str: + """Derive a page's URL path from its content path. + + Hugo bundles are the trap here: ``_index.md`` (branch) and ``index.md`` + (leaf) both publish at the containing directory's URL, so neither name + appears in the URL. + """ + rel = path[len(CONTENT) + 1:] + rel = re.sub(r"\.md$", "", rel) + rel = re.sub(r"/_?index$", "", rel) + return "" if rel in ("_index", "index") else rel + + +_render_never: set[str] | None = None + + +def render_never_roots() -> set[str]: + """Content directories Hugo is told never to render. + + ``content/embeds/_index.md`` sets ``build.render: never`` with a ``cascade``, + so none of the 119 fragment files beneath it is published -- they are pulled + in by the ``embed-yaml`` shortcode instead, and most have no frontmatter at + all, so there is nowhere to put an alias even if one were wanted. Derived + from the tree rather than hardcoded, so a new one is picked up for free. + """ + global _render_never + if _render_never is not None: + return _render_never + + import yaml + + roots: set[str] = set() + try: + candidates = git("grep", "-l", "render: never", "--", CONTENT).splitlines() + except subprocess.CalledProcessError: + candidates = [] # git grep exits 1 when nothing matches + for path in candidates: + if not path.endswith("_index.md"): + continue + try: + lines = read_lines(path) + except OSError: + continue + bounds = frontmatter_bounds(lines) + if not bounds: + continue + try: + data = yaml.safe_load("".join(lines[1:bounds[1]])) or {} + except yaml.YAMLError: + continue + if not isinstance(data, dict): + continue + cascade = data.get("cascade") or {} + build = cascade.get("build") if isinstance(cascade, dict) else None + if isinstance(build, dict) and str(build.get("render")) == "never": + roots.add(os.path.dirname(path)[len(CONTENT) + 1:] + "/") + _render_never = roots + return roots + + +_drafts: set[str] | None = None + + +def draft_paths() -> set[str]: + """Content files Hugo will not publish, because they are drafts. + + Production runs plain ``hugo`` with no ``--buildDrafts``, so a draft page + emits nothing at all -- **including its aliases**. That matters in both + directions: an alias added to a draft is silently a no-op, and treating a + draft as occupying a URL would wrongly suppress a real redirect. 31 files + today. Found by grep first so this does not parse all 5,867 content files. + """ + global _drafts + if _drafts is not None: + return _drafts + + import yaml + + found: set[str] = set() + try: + candidates = git("grep", "-l", "-E", r"^draft:[ \t]*true", "--", + CONTENT).splitlines() + except subprocess.CalledProcessError: + candidates = [] + for path in candidates: + try: + lines = read_lines(path) + except OSError: + continue + bounds = frontmatter_bounds(lines) + if not bounds: + continue + try: + data = yaml.safe_load("".join(lines[1:bounds[1]])) or {} + except yaml.YAMLError: + continue + if not isinstance(data, dict): + continue + keyed = {str(k).lower(): v for k, v in data.items()} + if keyed.get("draft") in (True, "true"): + found.add(path) + _drafts = found + return found + + +def is_published(path: str) -> bool: + """False for content Hugo never publishes as a page of its own.""" + rel = path[len(CONTENT) + 1:] + if any(d.startswith("_") for d in rel.split("/")[:-1]): + return False # e.g. the historical content/_embeds/ + return not any(rel.startswith(root) for root in render_never_roots()) + + +def is_versioned(path: str) -> bool: + return bool(VERSIONED.search(path)) + + +def eligible(path: str) -> bool: + return (path.startswith(CONTENT + "/") and path.endswith(".md") + and is_published(path) and not is_versioned(path)) + + +def norm(url: str) -> str: + return url.strip().strip("/").lower() + + +# --------------------------------------------------------------------------- # +# frontmatter +# --------------------------------------------------------------------------- # + +def frontmatter_bounds(lines: list[str]) -> tuple[int, int] | None: + """Return (first, last) line indices of the ``---`` fences, or None.""" + if not lines or lines[0].rstrip("\n") != "---": + return None + for i in range(1, len(lines)): + if lines[i].rstrip("\n") == "---": + return 0, i + return None + + +def read_lines(path: str) -> list[str]: + with open(path, encoding="utf-8") as handle: + return handle.readlines() + + +def declared_aliases(path: str) -> set[str]: + """Every alias the file declares, normalized for comparison. + + Parsed with PyYAML rather than by hand: the repo uses block lists, + single-line inline lists, and multi-line inline lists, and a regex that + misses one of them silently under-reports. + """ + try: + lines = read_lines(path) + except OSError: + return set() + bounds = frontmatter_bounds(lines) + if not bounds: + return set() + import yaml # local import: only the alias path needs it + + try: + data = yaml.safe_load("".join(lines[1:bounds[1]])) or {} + except yaml.YAMLError: + logger.warning(" ! %s: frontmatter is not valid YAML, skipping", path) + return set() + if not isinstance(data, dict): + return set() + # Hugo frontmatter keys are case-insensitive. + values = next((v for k, v in data.items() if str(k).lower() == "aliases"), None) + if values is None: + return set() + if isinstance(values, str): + # Hugo casts a scalar `aliases` value with cast.ToStringSlice, which runs + # strings.Fields, so a bare string is split on whitespace into several + # aliases. That is not what a YAML library does -- PyYAML folds + # + # aliases: /a/ + # /b/ + # + # into the single string "/a/ /b/" -- and taking the library's reading + # cost a false positive here, because Hugo publishes both of those as + # working aliases. Verified against Hugo 0.143.1. List items are *not* + # split, so only the scalar case gets this treatment. + values = values.split() + if not isinstance(values, list): + return set() + return {norm(str(v)) for v in values if v is not None and str(v).strip()} + + +# --------------------------------------------------------------------------- # +# finding moves +# --------------------------------------------------------------------------- # + +def order_renames(pending: list[tuple[str, str]]) -> list[tuple[str, str]]: + """Order one commit's renames so a chain resolves whatever order git listed. + + A commit can contain both A->B and B->C, and the chain only resolves if A->B + is applied first. git's ordering is not guaranteed to oblige, so an edge waits + while any other edge in the same commit still renames *into* its source. No + commit in this repo's history contains such a chain today, so this changes + nothing here -- but getting it wrong loses a move silently, which is the + failure mode this whole script exists to avoid. + """ + remaining = list(pending) + ordered: list[tuple[str, str]] = [] + while remaining: + ready = [(o, n) for o, n in remaining + if not any(n2 == o for o2, n2 in remaining if (o2, n2) != (o, n))] + if not ready: + ready = list(remaining) # a rename cycle; apply as listed and move on + ordered.extend(ready) + remaining = [e for e in remaining if e not in ready] + return ordered + + +def find_moves(rev_range: str | None, threshold: int) -> list[Move]: + """Renames in the given range, chained so each page resolves to its final home.""" + args = ["log", "--reverse", f"--find-renames={threshold}%", "--diff-filter=R", + "--name-status", "--format=COMMIT\t%H\t%ad", "--date=short"] + if rev_range: + args.append(rev_range) + args += ["--", CONTENT] + try: + out = git(*args) + except subprocess.CalledProcessError as exc: + logger.error("check_missing_aliases: git log failed for range %r.\n%s", + rev_range, exc.stderr.strip()) + raise + + # path-as-it-stands-now -> the (old_path, date, commit) records behind it + history: dict[str, set[tuple[str, str, str]]] = {} + edges: dict[str, str] = {} + pending: list[tuple[str, str]] = [] + commit = date = "" + + def absorb(old: str, new: str) -> None: + # Merge rather than assign. If a second file later renames onto a path + # that already carries a chain -- possible once the first occupant has + # been deleted rather than moved -- assigning would drop the earlier + # records silently. That happens once in this repo's history, and in a + # degenerate form where the dropped record has the same old_path, so + # merging changes nothing today. It is here because the loss would be + # invisible, and any real ambiguity it surfaces is caught downstream by + # the collision check rather than acted on. + carried = history.pop(old, set()) + history.setdefault(new, set()).update(carried) + history[new].add((old, date, commit)) + edges[old] = new + + def flush() -> None: + """Apply one commit's renames in dependency order, then clear the buffer.""" + for old, new in order_renames(pending): + absorb(old, new) + pending.clear() + + for line in out.splitlines(): + if line.startswith("COMMIT\t"): + flush() + _, commit, date = line.split("\t") + continue + parts = line.split("\t") + if len(parts) != 3 or not parts[0].startswith("R"): + continue + _, old, new = parts + if not (eligible(old) and eligible(new)): + continue + pending.append((old, new)) + flush() + + # A "demoting split" is X.md -> X/.md: a page broken up into a + # section, so its old URL becomes the section's URL while the file itself + # becomes one page inside it. Redirecting the old URL to that one child is + # usually wrong -- someone holding a link to the old landing page should + # arrive at the new landing page, not at whichever child inherited the file. + # A split to X/_index.md is fine and not counted, because the URL is + # unchanged. 6 such splits exist here, and 3 aliases reach a target through + # one; git records lineage, and lineage is not the same as equivalence, so + # these are reported for a person rather than guessed at. + demoting = {old for old, new in edges.items() + if os.path.dirname(new) + ".md" == old + and not new.endswith("_index.md")} + + def crosses_a_split(start: str) -> str: + seen: set[str] = set() + path = start + while path in edges and path not in seen: + seen.add(path) + if path in demoting: + return f"{path} -> {edges[path]}" + path = edges[path] + return "" + + tracked = set(git("ls-files", CONTENT).splitlines()) + moves: list[Move] = [] + for new_path, records in history.items(): + if new_path not in tracked: + continue # moved, then later deleted -- nothing to redirect to + new_url = to_url(new_path) + for old_path, date, commit in records: + old_url = to_url(old_path) + if norm(old_url) == norm(new_url): + continue # a bundle rename, or otherwise URL-preserving + moves.append(Move(old_path=old_path, new_path=new_path, + old_url=old_url, new_url=new_url, + date=date, commit=commit[:9], + split_at=crosses_a_split(old_path))) + + # A redirect is identified by where it comes from and where it goes, so the + # same pair reached by two routes -- a page moved away and back, or a + # recurring rename like the monthly changelog -- is one redirect, not two. + # 14 pairs in this repo's history arrive twice. Keep the earliest. + moves.sort(key=lambda m: (m.date, m.old_url)) + seen: set[tuple[str, str]] = set() + unique: list[Move] = [] + for move in moves: + fingerprint = (norm(move.old_url), move.new_path) + if fingerprint in seen: + continue + seen.add(fingerprint) + unique.append(move) + return unique + + +def published_urls() -> set[str]: + """Normalized URLs of every page published today. Drafts do not count.""" + drafts = draft_paths() + return {norm(to_url(p)) for p in git("ls-files", CONTENT).splitlines() + if eligible(p) and p not in drafts} + + +def alias_owners() -> dict[str, set[str]]: + """Every alias currently declared anywhere in content, mapped to its owners.""" + owners: dict[str, set[str]] = {} + try: + candidates = git("grep", "-l", "-E", "^aliases:", "--", CONTENT).splitlines() + except subprocess.CalledProcessError: + return owners + for path in candidates: + for alias in declared_aliases(path): + owners.setdefault(alias, set()).add(path) + return owners + + +def classify(moves: list[Move]) -> None: + """Mark each move as aliased, occupied by a live page, or colliding. + + A collision is the trap that has no safe automatic answer: Hugo resolves two + pages claiming the same alias by picking one and emitting a warning, so + adding the alias would quietly make the redirect ambiguous rather than fix + it. 28 of this repo's gaps are collisions -- 24 where another page already + claims the URL, and 4 where two moved pages both want it. + """ + current = published_urls() + owners = alias_owners() + drafts = draft_paths() + alias_cache: dict[str, set[str]] = {} + + for move in moves: + if move.new_path not in alias_cache: + alias_cache[move.new_path] = declared_aliases(move.new_path) + move.aliased = norm(move.old_url) in alias_cache[move.new_path] + move.occupied = norm(move.old_url) in current + move.target_draft = move.new_path in drafts + if not (move.aliased or move.occupied): + claimed = owners.get(norm(move.old_url), set()) - {move.new_path} + move.collides_with = sorted(claimed) + + # Two moved pages wanting the same alias collide with each other, which no + # amount of looking at existing frontmatter would reveal. + wanted: dict[str, set[str]] = {} + for move in moves: + if move.actionable: + wanted.setdefault(norm(move.old_url), set()).add(move.new_path) + for move in moves: + rivals = wanted.get(norm(move.old_url), set()) - {move.new_path} + if move.actionable and rivals: + move.collides_with = sorted(rivals) + + +# --------------------------------------------------------------------------- # +# --fix +# --------------------------------------------------------------------------- # + +def insert_aliases(lines: list[str], new_aliases: list[str]) -> list[str] | None: + """Add aliases to a file's frontmatter, editing line by line. + + Deliberately not a YAML round-trip: ``yaml.safe_load`` followed by + ``yaml.dump`` reorders keys alphabetically and renormalizes quoting, which + would rewrite the frontmatter of every file it touched into an unreviewable + diff. This preserves everything it does not need to change. + """ + bounds = frontmatter_bounds(lines) + if not bounds: + return None + _, close = bounds + + key = None + for i in range(1, close): + if re.match(r"(?i)aliases[ \t]*:", lines[i]): + key = i + break + + if key is None: + # No aliases key at all: add one just above the closing fence. + block = ["aliases:\n"] + [f"- {a}\n" for a in new_aliases] + return lines[:close] + block + lines[close:] + + rest = lines[key].split(":", 1)[1].strip() + indent = re.match(r"[ \t]*", lines[key]).group(0) + + if rest.startswith("[") and rest.endswith("]") and len(rest) > 1: + # Single-line inline list: aliases: [/a/, /b/] + inner = rest[1:-1].strip().rstrip(",").strip() + items = ([inner] if inner else []) + new_aliases + lines = list(lines) + lines[key] = f"{indent}aliases: [{', '.join(items)}]\n" + return lines + + if rest == "[": + # Multi-line inline list: find its closing bracket. + for j in range(key + 1, close): + if lines[j].strip().startswith("]"): + item_indent = (re.match(r"[ \t]*", lines[key + 1]).group(0) + if j > key + 1 else indent + " ") + block = [f"{item_indent}{a},\n" for a in new_aliases] + return lines[:j] + block + lines[j:] + return None + + # A YAML folded scalar continued on the next line: + # + # aliases: /a/ + # /b/ + # + # This is valid YAML but reads as the single string "/a/ /b/", so the + # author's second alias never worked. Editing only the first line would + # leave the continuation dangling and break the frontmatter outright, so + # refuse it and let a human fix the underlying content bug. One file today. + following = lines[key + 1] if key + 1 < close else "" + if (following.strip() # a blank line is not a continuation + and following[:1] in (" ", "\t") + and not re.match(r"[ \t]*-[ \t]*\S", following) + and not re.match(r"[ \t]*\S+[ \t]*:", following)): + return None + + if rest.lower() in NO_VALUE: + # A block list, or the key with no value. 49 files write `aliases: null` + # and 39 leave it bare; YAML reads both as absent, so the placeholder is + # dropped rather than carried into the list as a literal "null" entry. + lines = list(lines) + if rest: + lines[key] = f"{indent}aliases:\n" + last = key + for j in range(key + 1, close): + if re.match(r"[ \t]*-[ \t]*\S", lines[j]): + last = j + elif lines[j].strip() == "": + continue + else: + break + item_indent = (re.match(r"[ \t]*", lines[last]).group(0) + if last != key else indent) + block = [f"{item_indent}- {a}\n" for a in new_aliases] + return lines[:last + 1] + block + lines[last + 1:] + + # A scalar value (aliases: /a/) -- 104 files. Promote it to a *block* list, + # never an inline one, and keep each existing token byte-for-byte. + # + # Inline promotion silently changes what the page publishes. One file holds + # `aliases: /operate/kubernetes/release-notes/7-4-6-2, ` -- an author writing + # a list without brackets -- and Hugo publishes an alias whose path ends in a + # comma, which is live and returns 200 today. Written inline, that comma + # becomes the list separator and the alias silently changes to the + # comma-free path, turning a working URL into a 404. A block list has no + # separator to be confused with, so the value survives exactly. + # + # Splitting on whitespace matches Hugo's cast.ToStringSlice, so a scalar + # holding several aliases becomes several list items rather than one. + lines = list(lines) + lines[key] = f"{indent}aliases:\n" + items = rest.split() + new_aliases + block = [f"{indent}- {a}\n" for a in items] + return lines[:key + 1] + block + lines[key + 1:] + + +def apply_fixes(moves: list[Move]) -> tuple[int, int, list[str]]: + """Write missing aliases into frontmatter. + + Returns (files changed, aliases added, files that still need a manual fix). + The third value matters to callers: a sweep that reports success while some + aliases could not be placed would claim a complete fix it did not make. + """ + by_file: dict[str, FileFix] = {} + for move in moves: + if not move.actionable: + continue + fix = by_file.setdefault(move.new_path, FileFix(path=move.new_path)) + alias = ALIAS_TEMPLATE.format(url=norm(move.old_url)) + if alias not in fix.aliases: + fix.aliases.append(alias) + + files = aliases = 0 + skipped: list[str] = [] + for fix in by_file.values(): + if not os.path.exists(fix.path): + logger.warning(" ! %s no longer exists, skipping", fix.path) + skipped.append(fix.path) + continue + existing = declared_aliases(fix.path) + wanted = [a for a in fix.aliases if norm(a) not in existing] + if not wanted: + continue + lines = read_lines(fix.path) + updated = insert_aliases(lines, wanted) + if updated is None: + logger.warning(" ! %s: could not place aliases, skipping", fix.path) + skipped.append(fix.path) + continue + with open(fix.path, "w", encoding="utf-8") as handle: + handle.writelines(updated) + files += 1 + aliases += len(wanted) + logger.info(" + %s", fix.path) + for alias in wanted: + logger.info(" %s", alias) + return files, aliases, skipped + + +# --------------------------------------------------------------------------- # +# reporting +# --------------------------------------------------------------------------- # + +def report(moves: list[Move], github: bool, fix_hint: str) -> list[Move]: + missing = [m for m in moves if m.actionable] + occupied = [m for m in moves if not m.aliased and m.occupied] + drafted = [m for m in moves + if not m.aliased and not m.occupied and m.target_draft] + splits = [m for m in moves if not m.aliased and not m.occupied + and not m.target_draft and m.split_at] + collisions = [m for m in moves if not m.aliased and not m.occupied + and not m.target_draft and not m.split_at and m.collides_with] + aliased = [m for m in moves if m.aliased] + + logger.info("check_missing_aliases: %d URL-changing move(s) found.", len(moves)) + if moves: + logger.info(" %d already aliased, %d missing an alias, %d skipped " + "(old URL is a live page), %d skipped (target is a draft), " + "%d need a decision (page split), %d need a decision " + "(collision).", + len(aliased), len(missing), len(occupied), len(drafted), + len(splits), len(collisions)) + + if occupied: + logger.info("Skipped -- old URL currently resolves, so must not redirect:") + for move in occupied: + logger.info(" %s %s", move.date, move.old_url) + + if drafted: + logger.info("Skipped -- the page moved to is a draft, so it publishes " + "nothing and an alias on it would do nothing:") + for move in drafted: + logger.info(" %s %s -> %s", move.date, move.old_url, move.new_path) + + if splits: + logger.warning("Needs a human decision -- the old page was split into a " + "section, so the right target is probably its new landing " + "page rather than the child that inherited the file:") + for move in splits: + logger.warning(" %s %s", move.date, move.old_url) + logger.warning(" lineage ends at %s", move.new_url) + logger.warning(" split at %s", move.split_at) + + if collisions: + logger.warning("Needs a human decision -- another page already claims " + "this URL, so Hugo would pick one arbitrarily:") + for move in collisions: + logger.warning(" %s %s", move.date, move.old_url) + logger.warning(" wanted by %s", move.new_path) + for owner in move.collides_with: + logger.warning(" claimed by %s", owner) + + if missing: + logger.warning("Moved with no alias for the old URL:") + for move in missing: + logger.warning(" %s %s %s", move.date, move.commit, move.old_url) + logger.warning(" now at %s", move.new_url) + logger.warning(" add to %s: %s", move.new_path, + ALIAS_TEMPLATE.format(url=norm(move.old_url))) + if github: + print(f"::warning file={move.new_path}::Page moved from " + f"/{norm(move.old_url)}/ with no alias. Add " + f"'{ALIAS_TEMPLATE.format(url=norm(move.old_url))}' to its " + f"aliases, or run: make check_aliases_fix") + logger.warning("Fix them all with: %s", fix_hint) + return missing + + +def main() -> int: + logging.basicConfig(level=logging.INFO, format="%(message)s") + args = parse_args() + + rev_range = None if args.all else args.rev_range + try: + moves = find_moves(rev_range, args.threshold) + except subprocess.CalledProcessError: + return EXIT_ERROR + classify(moves) + + fix_hint = ("make check_aliases_fix" if args.all else + "python3 build/check_missing_aliases.py " + f"--range {args.rev_range} --fix") + missing = report(moves, args.github, fix_hint) + + if args.json_out: + with open(args.json_out, "w", encoding="utf-8") as handle: + json.dump([m.__dict__ for m in moves], handle, indent=1) + logger.info("Wrote %s", args.json_out) + + if args.fix and missing: + logger.info("Adding %d alias(es):", len(missing)) + files, aliases, skipped = apply_fixes(moves) + logger.info("check_missing_aliases: added %d alias(es) across %d file(s).", + aliases, files) + if skipped: + logger.warning("check_missing_aliases: could not place aliases in %d " + "file(s), which still need fixing by hand:", len(skipped)) + for path in skipped: + logger.warning(" %s", path) + return EXIT_FINDINGS if (skipped and args.fail) else EXIT_OK + + return EXIT_FINDINGS if (missing and args.fail) else EXIT_OK + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/build/test_check_missing_aliases.py b/build/test_check_missing_aliases.py new file mode 100644 index 0000000000..1b89d27149 --- /dev/null +++ b/build/test_check_missing_aliases.py @@ -0,0 +1,472 @@ +#!/usr/bin/env python3 +"""Tests for check_missing_aliases. + +The interesting logic is ``insert_aliases``, which edits frontmatter line by +line rather than round-tripping the YAML. It has to cope with every alias shape +already in the repo -- block lists (490 files), bare scalars (104), the key +spelled ``null`` (49), single-line inline lists (44), the key left bare (39), +multi-line inline lists (19), one folded multi-line scalar, and no key at all -- +while leaving every other line untouched. + +Run with ``pytest build/test_check_missing_aliases.py`` or directly. +""" + +import os +import sys + +sys.path.insert(0, os.path.dirname(__file__)) + +from check_missing_aliases import ( # noqa: E402 + Move, declared_aliases, draft_paths, eligible, insert_aliases, is_published, + is_versioned, norm, order_renames, published_urls, render_never_roots, to_url, +) + + +def apply(text: str, aliases: list) -> str: + result = insert_aliases(text.splitlines(keepends=True), aliases) + assert result is not None, "insert_aliases refused the frontmatter" + return "".join(result) + + +# --------------------------------------------------------------------------- # +# path -> URL +# --------------------------------------------------------------------------- # + +def test_to_url_strips_both_bundle_names(): + # The trap that produced 519 false positives: both bundle names publish at + # the containing directory's URL, so renaming between them changes nothing. + assert to_url("content/commands/lpushx/index.md") == "commands/lpushx" + assert to_url("content/commands/lpushx.md") == "commands/lpushx" + strings = "develop/data-types/strings" + assert to_url("content/develop/data-types/strings/_index.md") == strings + assert to_url("content/develop/data-types/strings.md") == strings + + +def test_to_url_handles_site_root(): + assert to_url("content/_index.md") == "" + + +def test_underscore_directories_are_not_published(): + assert not is_published("content/_embeds/k8s/rerc.md") + assert is_published("content/develop/clients/observability.md") + + +def test_render_never_cascade_is_read_off_the_tree(): + # content/embeds/_index.md sets build.render: never with a cascade, so its + # 119 fragment files are not pages. The underscore heuristic alone misses + # this directory because its name has no underscore. + assert "embeds/" in render_never_roots() + assert not is_published("content/embeds/k8s/rec.md") + assert not is_published("content/embeds/_index.md") + + +def test_versioned_paths_are_detected(): + assert is_versioned("content/operate/rs/7.8/references/rest-api.md") + assert is_versioned("content/develop/ai/redisvl/0.10.0/api/cache.md") + assert not is_versioned("content/operate/rs/references/rest-api.md") + + +def test_eligible_rejects_the_excluded_classes(): + assert eligible("content/develop/ai/langcache/_index.md") + assert not eligible("content/_embeds/k8s/rerc.md") + assert not eligible("content/operate/rs/7.8/index.md") + assert not eligible("content/develop/ai/langcache/api-reference/api.yaml") + + +def test_only_safe_moves_are_actionable(): + def move(**kwargs): + return Move(old_path="content/a.md", new_path="content/b.md", + old_url="a", new_url="b", date="2026-01-01", commit="abc", + **kwargs) + + assert move().actionable + assert not move(aliased=True).actionable + assert not move(occupied=True).actionable + # A collision has no safe automatic answer: Hugo would pick one of the two + # claimants arbitrarily, so the alias must not be added unattended. + assert not move(collides_with=["content/other.md"]).actionable + # A draft publishes nothing, aliases included, so writing one is a no-op. + assert not move(target_draft=True).actionable + + +def test_renames_in_one_commit_are_ordered_so_chains_resolve(): + # A commit holding both A->B and B->C only resolves if A->B goes first. + # git's listing order is not guaranteed to oblige, and getting it wrong + # loses the A move silently. + ab, bc = ("content/a.md", "content/b.md"), ("content/b.md", "content/c.md") + assert order_renames([bc, ab]) == [ab, bc] + assert order_renames([ab, bc]) == [ab, bc] + # Independent renames keep their order and none are dropped. + xy, pq = ("content/x.md", "content/y.md"), ("content/p.md", "content/q.md") + assert order_renames([xy, pq]) == [xy, pq] + # A cycle must terminate rather than spin, and must not lose an edge. + cycle = [("content/a.md", "content/b.md"), ("content/b.md", "content/a.md")] + assert sorted(order_renames(cycle)) == sorted(cycle) + # A three-link chain listed backwards. + cd = ("content/c.md", "content/d.md") + assert order_renames([cd, bc, ab]) == [ab, bc, cd] + + +def test_a_move_split_into_a_section_is_not_auto_fixed(): + def move(**kwargs): + return Move(old_path="content/a.md", new_path="content/b/c.md", + old_url="a", new_url="b/c", date="2026-01-01", commit="abc", + **kwargs) + + # git says this file descends from the old page, but the old URL was a + # landing page and its lineage ends at one child, so a person decides. + assert not move(split_at="content/a.md -> content/a/c.md").actionable + assert move().actionable + + +def test_a_whitespace_only_line_is_not_a_folded_continuation(): + # The folded-scalar guard must not be tripped by trailing whitespace on the + # line after a perfectly ordinary single-value scalar. + before = "---\naliases: /old/thing/\n \ntitle: T\n---\nBody.\n" + after = insert_aliases(before.splitlines(keepends=True), ["/new/thing/"]) + assert after is not None, "a blank line should not block the fix" + joined = "".join(after) + assert "- /old/thing/\n" in joined + assert "- /new/thing/\n" in joined + + +def test_drafts_are_detected_and_excluded_from_published_urls(): + drafts = draft_paths() + assert drafts, "expected this repo to contain drafts" + assert all(p.startswith("content/") and p.endswith(".md") for p in drafts) + # The draft that made this trap visible: Hugo declined to emit its two + # alias stubs during a full build, because the page itself is a draft. + assert "content/integrate/write-behind/_index.md" in drafts + published = published_urls() + assert norm(to_url("content/integrate/write-behind/_index.md")) not in published + + +# --------------------------------------------------------------------------- # +# insert_aliases -- one test per shape found in the repo +# --------------------------------------------------------------------------- # + +def test_block_list_appends_after_last_item(): + before = """--- +title: Bitmaps +aliases: +- /data-types/bitmaps/ +- /manual/data-types/bitmaps/ +weight: 10 +--- + +Body text. +""" + after = apply(before, ["/develop/data-types/bitmaps/"]) + assert after == """--- +title: Bitmaps +aliases: +- /data-types/bitmaps/ +- /manual/data-types/bitmaps/ +- /develop/data-types/bitmaps/ +weight: 10 +--- + +Body text. +""" + + +def test_single_line_inline_list_grows_in_place(): + before = """--- +title: Architecture +aliases: [/operate/kubernetes/architecture/] +weight: 5 +--- +Body. +""" + after = apply(before, ["/kubernetes/architecture/"]) + assert ("aliases: [/operate/kubernetes/architecture/, " + "/kubernetes/architecture/]\n") in after + assert "weight: 5\n" in after + + +def test_multi_line_inline_list_gains_a_line_before_the_bracket(): + before = """--- +title: Delete custom resources +aliases: [ + /operate/kubernetes/re-clusters/delete-custom-resources/, +] +weight: 7 +--- +Body. +""" + after = apply(before, ["/kubernetes/delete-custom-resources/"]) + assert after == """--- +title: Delete custom resources +aliases: [ + /operate/kubernetes/re-clusters/delete-custom-resources/, + /kubernetes/delete-custom-resources/, +] +weight: 7 +--- +Body. +""" + + +def test_empty_aliases_key_gains_the_first_item(): + # 39 files in the repo leave the key bare like this. + before = """--- +aliases: +categories: +- docs +title: Quantization +--- +Body. +""" + after = apply(before, ["/develop/ai/search-and-query/vectors/svs-compression/"]) + assert after == """--- +aliases: +- /develop/ai/search-and-query/vectors/svs-compression/ +categories: +- docs +title: Quantization +--- +Body. +""" + + +def test_explicit_null_is_dropped_not_kept_as_an_item(): + # 49 files spell the empty key `aliases: null`. An earlier version promoted + # it to `[null, /new/]`, which would have published an alias called "null". + before = """--- +aliases: null +title: Data transformation +--- +Body. +""" + after = apply(before, ["/integrate/redis-data-integration/data-transformation/"]) + assert after == """--- +aliases: +- /integrate/redis-data-integration/data-transformation/ +title: Data transformation +--- +Body. +""" + assert "null" not in after + + +def test_scalar_value_is_promoted_to_a_block_list(): + before = """--- +aliases: /develop/connect/clients/dotnet +title: .NET +--- +Body. +""" + after = apply(before, ["/develop/clients/dotnet/"]) + assert after == """--- +aliases: +- /develop/connect/clients/dotnet +- /develop/clients/dotnet/ +title: .NET +--- +Body. +""" + + +def test_scalar_with_a_trailing_comma_keeps_the_comma(): + """A live URL must not change because we tidied its frontmatter. + + Real frontmatter in the repo: an author wrote a list without brackets, so + Hugo publishes an alias whose path ends in a comma. That URL returns 200 + today. Promoted to an *inline* list the comma becomes the separator and the + alias silently changes to the comma-free path -- observed as a lost page when + diffing two full builds. A block list preserves it. + """ + before = """--- +weight: 29 +aliases: /operate/kubernetes/release-notes/7-4-6-2, +--- +Body. +""" + after = apply(before, ["/operate/kubernetes/release-notes/7-4-6-2/"]) + assert "- /operate/kubernetes/release-notes/7-4-6-2,\n" in after + assert "- /operate/kubernetes/release-notes/7-4-6-2/\n" in after + assert "aliases: [" not in after + + +def test_folded_scalar_promotion_keeps_both_aliases(): + # Hugo reads a folded scalar as two aliases, so promotion must emit two + # items rather than one item containing a space. + before = """--- +aliases: /operate/search/scalable-search/ + /operate/search/query-performance-factor/ +weight: 20 +--- +Body. +""" + result = insert_aliases(before.splitlines(keepends=True), ["/new/"]) + # The folded form is refused outright, so nothing is silently mangled. + assert result is None + + +def test_missing_key_is_added_above_the_closing_fence(): + before = """--- +Title: Redis Agent Memory +linkTitle: Agent Memory +weight: 20 +--- + +Give your AI agents persistent memory. +""" + after = apply(before, ["/develop/ai/agent-memory/"]) + assert after == """--- +Title: Redis Agent Memory +linkTitle: Agent Memory +weight: 20 +aliases: +- /develop/ai/agent-memory/ +--- + +Give your AI agents persistent memory. +""" + + +def test_several_aliases_are_added_at_once(): + before = """--- +title: Thing +aliases: +- /old/one/ +--- +Body. +""" + after = apply(before, ["/old/two/", "/old/three/"]) + assert after.count("- /old/") == 3 + assert after.index("/old/two/") < after.index("/old/three/") + + +def test_body_is_never_touched(): + # A body containing something that looks like frontmatter must survive. + before = """--- +title: Thing +weight: 1 +--- + +Some prose. + +--- + +aliases: not-really-frontmatter + +More prose. +""" + after = apply(before, ["/old/thing/"]) + assert after.endswith("aliases: not-really-frontmatter\n\nMore prose.\n") + assert after.count("aliases:") == 2 + + +def test_no_frontmatter_is_refused_rather_than_guessed(): + assert insert_aliases(["Just a body.\n"], ["/old/"]) is None + + +def test_folded_multiline_scalar_is_refused(): + # Real frontmatter in the repo. Valid YAML, but it folds to the single + # string "/a/ /b/" so the second alias never worked. Rewriting only the + # first line would leave the continuation dangling and break the file, so + # the fixer must decline rather than guess the author's intent. + before = """--- +weight: 20 +aliases: /operate/search/scalable-search/ + /operate/search/query-performance-factor/ +--- +Body. +""" + assert insert_aliases(before.splitlines(keepends=True), ["/new/"]) is None + + +# --------------------------------------------------------------------------- # +# declared_aliases -- parsing every shape back out again +# --------------------------------------------------------------------------- # + +def test_declared_aliases_reads_every_shape(): + import tempfile + + shapes = { + "block": "---\naliases:\n- /a/\n- /b/\n---\nx\n", + "inline": "---\naliases: [/a/, /b/]\n---\nx\n", + "multiline": "---\naliases: [\n /a/,\n /b/,\n]\n---\nx\n", + "scalar": "---\naliases: /a/\n---\nx\n", + "empty": "---\naliases:\n---\nx\n", + "absent": "---\ntitle: t\n---\nx\n", + "uppercase": "---\nAliases:\n- /a/\n---\nx\n", + } + expected = { + "block": {"a", "b"}, "inline": {"a", "b"}, "multiline": {"a", "b"}, + "scalar": {"a"}, "empty": set(), "absent": set(), "uppercase": {"a"}, + } + with tempfile.TemporaryDirectory() as tmp: + for name, text in shapes.items(): + path = os.path.join(tmp, f"{name}.md") + with open(path, "w", encoding="utf-8") as handle: + handle.write(text) + assert declared_aliases(path) == expected[name], name + + +def test_scalar_aliases_are_split_on_whitespace_like_hugo(): + """Hugo casts a scalar `aliases` with cast.ToStringSlice, i.e. strings.Fields. + + So a folded multi-line scalar publishes *two* working aliases, even though + PyYAML reads it as the single string "/a/ /b/". Trusting the YAML library + here produced a false positive against a page whose aliases both work. + Verified against Hugo 0.143.1. + """ + import tempfile + + folded = """--- +title: QPF +aliases: /operate/search/scalable-search/ + /operate/search/query-performance-factor/ +--- +body +""" + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "folded.md") + with open(path, "w", encoding="utf-8") as handle: + handle.write(folded) + assert declared_aliases(path) == { + "operate/search/scalable-search", + "operate/search/query-performance-factor", + } + # A single-valued scalar must still read as exactly one alias. + with open(path, "w", encoding="utf-8") as handle: + handle.write("---\naliases: /a/b/\n---\nx\n") + assert declared_aliases(path) == {"a/b"} + + +def test_round_trip_every_shape(): + """Whatever we insert must be readable back as an alias.""" + import tempfile + + shapes = [ + "---\naliases:\n- /a/\n---\nx\n", + "---\naliases: [/a/]\n---\nx\n", + "---\naliases: [\n /a/,\n]\n---\nx\n", + "---\naliases:\ntitle: t\n---\nx\n", + "---\naliases: null\ntitle: t\n---\nx\n", + "---\naliases: /a/\ntitle: t\n---\nx\n", + "---\naliases: /a/, \ntitle: t\n---\nx\n", + "---\ntitle: t\n---\nx\n", + ] + with tempfile.TemporaryDirectory() as tmp: + for i, text in enumerate(shapes): + path = os.path.join(tmp, f"s{i}.md") + with open(path, "w", encoding="utf-8") as handle: + handle.write(apply(text, ["/new/one/"])) + assert "new/one" in declared_aliases(path), text + + +if __name__ == "__main__": + failures = 0 + for name, fn in sorted(list(globals().items())): + if name.startswith("test_") and callable(fn): + try: + fn() + print(f" ok {name}") + except AssertionError as exc: + failures += 1 + print(f" FAIL {name}: {exc}") + print(f"\n{failures} failure(s)") + sys.exit(1 if failures else 0)