From c166c498ede082fe098617e0dcf78142c99ac079 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Thu, 24 Sep 2026 09:45:16 +0100 Subject: [PATCH 1/2] DOC-7104: Add a check for links left in bare, uncanonicalized form migrate_shortcode_links.py's linkify stage correctly canonicalizes almost every converted link to /content/.md[#anchor], but a link that gets a manual post-hoc text fix after the pipeline already ran (e.g. inserting a separator slash a relref-plus-literal-suffix concatenation was missing) never gets a second pass through linkify. The result is a bare /operate/... path that resolves to the exact same rendered href as the canonical form, so build/diff_rendered_hrefs.py -- this migration's usual verification -- is blind to it by construction. Found on PR #4086 (DOC-7104 release-notes/ unit): human review manually flagged 8 malformed links; a corpus-wide grep for the same shape found 21 across 9 files (13 more than manual review caught), plus 8 more that turned out to be genuinely pre-existing dead links in the identical shape, invisible to any prior check since Hugo's relref shortcode only ever validated its own target, never text concatenated onto it afterward. check_uncanonicalized_links.py reuses migrate_shortcode_links.py's own resolver (_find_content_file) so a --fix run applies the exact same rewrite the pipeline would have. Verified against the pre-fix state of PR #4086: reproduces the same 21 FIXABLE / 8 DEAD split exactly, and a full-corpus scan of content/ elsewhere comes back to 3 unrelated hits, confirming it isn't noisy. Co-Authored-By: Claude Sonnet 5 --- build/check_uncanonicalized_links.py | 164 ++++++++++++++++++++++ build/test_check_uncanonicalized_links.py | 135 ++++++++++++++++++ 2 files changed, 299 insertions(+) create mode 100644 build/check_uncanonicalized_links.py create mode 100644 build/test_check_uncanonicalized_links.py diff --git a/build/check_uncanonicalized_links.py b/build/check_uncanonicalized_links.py new file mode 100644 index 0000000000..5bb338785a --- /dev/null +++ b/build/check_uncanonicalized_links.py @@ -0,0 +1,164 @@ +#!/usr/bin/env python3 +"""Report Markdown links whose target lives under a real content mount +(/operate/, /develop/, /integrate/, /commands/) but was never rewritten to +this migration's canonical `/content/.md[#anchor]` form. + +Why this exists (DOC-7104, PR #4086 review): `build/migrate_shortcode_links.py` +correctly canonicalizes almost everything, but a hand-fix applied AFTER its +pipeline ran -- inserting a separator slash a relref-plus-literal-suffix +concatenation was missing -- produces a syntactically fine bare path that +never gets a second pass through `linkify`. The bare path resolves to the +exact same rendered href as the canonical form, so +`build/diff_rendered_hrefs.py` (this migration's usual verification) is +BLIND to the defect by construction -- it only surfaced because a human +reviewer read the diff and recognized the shape, and even then caught 8 of +the 21 real instances in that one PR. This is the automated backstop: it +does not care WHY a link ended up bare (this concatenation idiom, a manual +fix, a future gap), only that it did. + +Three outcomes per candidate link, and why each is handled differently: + * FIXABLE -- resolves to a real content file via the exact same resolver + `migrate_shortcode_links.py` uses (`_find_content_file`, no module-mount + remapping). `--fix` rewrites it to the canonical form; without `--fix` + it's reported so a human can choose to apply it. + * MOUNT_ONLY -- doesn't resolve directly, but resolves once module mounts + are followed (`check_shortcode_paths.resolve_relref`). The one real + mount (rs/rc active-active) has an unresolved product question about + which permalink is "correct" (see `migrate_shortcode_links.py`'s + `_find_content_file` docstring) -- baking in a guess here would be + exactly the kind of wrong rewrite this whole migration avoids. Reported, + never auto-fixed. + * DEAD -- doesn't resolve at all, by any route. A pre-existing broken + link, invisible to every check before this one: Hugo's `relref` + shortcode only ever validated its OWN target, never text a page + author concatenated onto it afterward. Reported only -- guessing the + intended real target is a content-fact decision, not a mechanical fix. + +`/commands` and `/commands/` are a fourth, silent case: the commands index +is templated with no backing `_index.md` (a known, accepted gap -- see +project memory reference_commands_group_link_convention), so a bare +`/commands/?group=x` link is correct AS WRITTEN and not a finding. + +Usage: + build/check_uncanonicalized_links.py ... + build/check_uncanonicalized_links.py --fix ... + +Exit status: 1 if any FIXABLE or DEAD finding remains after running (0 if +--fix cleared every FIXABLE one and only MOUNT_ONLY findings remain, or if +there's nothing to report). MOUNT_ONLY alone does not fail the run -- it's +a known, deliberately-unresolved category, not a defect this script expects +to be zero. +""" + +import sys +import os +import re +import argparse + +sys.path.insert(0, os.path.dirname(__file__)) +import migrate_shortcode_links as msl # noqa: E402 + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", ".claude", "hooks")) +import check_shortcode_paths as csp # noqa: E402 + +MOUNT_PREFIX_RX = re.compile(r'^/(operate|develop|integrate|commands)(/|$)') + + +def find_root(start): + return csp.find_root(start) or os.getcwd() + + +def iter_markdown_files(paths): + for p in paths: + if os.path.isdir(p): + for dirpath, _dirs, filenames in os.walk(p): + for name in filenames: + if name.endswith(".md"): + yield os.path.join(dirpath, name) + elif p.endswith(".md"): + yield p + + +def check_file(path, root): + """Return a list of (category, line_no, old_href, new_href_or_None).""" + text = open(path, encoding="utf-8").read() + findings = [] + for m in msl.LINK_RX.finditer(text): + href = m.group(1) + if href.startswith(msl.SKIP_HREF_PREFIXES): + continue + if not MOUNT_PREFIX_RX.match(href): + continue + + split = re.search(r"[#?]", href) + base_ref, suffix = (href[: split.start()], href[split.start() :]) if split else (href, "") + + bare = base_ref.rstrip("/") + if bare in ("/commands",): + continue # templated index, no backing file by design + + line_no = text.count("\n", 0, m.start()) + 1 + target = msl._find_content_file(root, base_ref) + if target is not None: + findings.append(("FIXABLE", line_no, href, f"/{target}{suffix}")) + continue + if csp.resolve_relref(root, base_ref): + findings.append(("MOUNT_ONLY", line_no, href, None)) + continue + findings.append(("DEAD", line_no, href, None)) + return findings + + +def apply_fixes(path, findings): + fixable = [f for f in findings if f[0] == "FIXABLE"] + if not fixable: + return 0 + text = open(path, encoding="utf-8").read() + for _cat, _line, old_href, new_href in fixable: + old_link = "](" + old_href + ")" + new_link = "](" + new_href + ")" + text = text.replace(old_link, new_link) + open(path, "w", encoding="utf-8").write(text) + return len(fixable) + + +def main(argv): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("paths", nargs="+", help="files or directories to scan") + parser.add_argument("--fix", action="store_true", help="rewrite FIXABLE links in place") + args = parser.parse_args(argv) + + root = find_root(args.paths[0]) + files = sorted(set(iter_markdown_files(args.paths))) + if not files: + print("No .md files found under the given paths.") + return 1 + + totals = {"FIXABLE": 0, "MOUNT_ONLY": 0, "DEAD": 0} + fixed = 0 + for path in files: + findings = check_file(path, root) + if not findings: + continue + if args.fix: + fixed += apply_fixes(path, findings) + for cat, line_no, old_href, new_href in findings: + totals[cat] += 1 + if cat == "FIXABLE": + arrow = " -> " + new_href if args.fix else " (would rewrite to " + new_href + ")" + print(f"{cat:10} {path}:{line_no} {old_href}{arrow}") + else: + print(f"{cat:10} {path}:{line_no} {old_href}") + + print( + f"\n{len(files)} files scanned. " + f"FIXABLE={totals['FIXABLE']} MOUNT_ONLY={totals['MOUNT_ONLY']} DEAD={totals['DEAD']}" + + (f" (fixed {fixed})" if args.fix else "") + ) + if args.fix: + return 1 if totals["DEAD"] else 0 + return 1 if (totals["FIXABLE"] or totals["DEAD"]) else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/build/test_check_uncanonicalized_links.py b/build/test_check_uncanonicalized_links.py new file mode 100644 index 0000000000..ae29013d10 --- /dev/null +++ b/build/test_check_uncanonicalized_links.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python3 +"""Tests for check_uncanonicalized_links. + +Builds a minimal real content/ tree per test (no mocking), matching this +repo's build/test_*.py convention, so resolution goes through the actual +filesystem the way it does against the real corpus. + +Run with ``pytest build/test_check_uncanonicalized_links.py`` or directly. +""" + +import os +import sys +import tempfile + +sys.path.insert(0, os.path.dirname(__file__)) + +from check_uncanonicalized_links import check_file # noqa: E402 + + +def make_tree(tmp, pages): + """pages: {relpath-under-content: text}. Returns tmp (the Hugo root).""" + os.makedirs(os.path.join(tmp, "layouts"), exist_ok=True) + for relpath, text in pages.items(): + full = os.path.join(tmp, "content", relpath) + os.makedirs(os.path.dirname(full), exist_ok=True) + with open(full, "w", encoding="utf-8") as f: + f.write(text) + return tmp + + +def run(pages, target_relpath): + with tempfile.TemporaryDirectory() as tmp: + root = make_tree(tmp, pages) + return check_file(os.path.join(root, "content", target_relpath), root) + + +def test_bare_path_to_real_page_is_fixable(): + findings = run( + { + "operate/rs/security/access-control.md": "x", + "operate/rs/release-notes/foo.md": "See [ACL](/operate/rs/security/access-control) for details.", + }, + "operate/rs/release-notes/foo.md", + ) + assert len(findings) == 1 + cat, line_no, old, new = findings[0] + assert cat == "FIXABLE" + assert old == "/operate/rs/security/access-control" + assert new == "/content/operate/rs/security/access-control.md" + + +def test_bare_path_with_anchor_preserves_anchor(): + findings = run( + { + "operate/rs/security/access-control.md": "x", + "operate/rs/release-notes/foo.md": "See [ACL](/operate/rs/security/access-control/#some-heading).", + }, + "operate/rs/release-notes/foo.md", + ) + assert len(findings) == 1 + assert findings[0][3] == "/content/operate/rs/security/access-control.md#some-heading" + + +def test_dead_link_reported_not_guessed(): + findings = run( + { + "operate/rs/release-notes/foo.md": "See [ghost](/operate/rs/nonexistent/page/) for details.", + }, + "operate/rs/release-notes/foo.md", + ) + assert len(findings) == 1 + cat, _line, old, new = findings[0] + assert cat == "DEAD" + assert new is None + + +def test_already_canonical_link_is_not_a_finding(): + findings = run( + { + "operate/rs/release-notes/foo.md": "See [ACL](/content/operate/rs/security/access-control.md).", + }, + "operate/rs/release-notes/foo.md", + ) + assert findings == [] + + +def test_external_and_anchor_only_links_are_not_findings(): + findings = run( + { + "operate/rs/release-notes/foo.md": ( + "[ext](https://example.com/operate/rs/) " + "[frag](#section) " + "[mail](mailto:a@example.com)" + ), + }, + "operate/rs/release-notes/foo.md", + ) + assert findings == [] + + +def test_bare_commands_index_with_group_query_is_not_a_finding(): + # /commands has no backing _index.md by design (templated index) -- + # this is the one accepted bare-path exception. + findings = run( + { + "operate/rs/release-notes/foo.md": "See [commands](/commands/?group=cluster).", + }, + "operate/rs/release-notes/foo.md", + ) + assert findings == [] + + +def test_relref_plus_suffix_shape_matches_the_pr_4086_case(): + # The actual defect this script was written for: a relref-plus-literal- + # suffix concatenation, already unwrapped and slash-fixed by hand, that + # never got a second pass through linkify. + findings = run( + { + "operate/rs/databases/active-active/develop/develop-for-aa.md": "x", + "operate/rs/release-notes/foo.md": ( + "[Developing with CRDBs](/operate/rs/databases/active-active/develop/develop-for-aa/)." + ), + }, + "operate/rs/release-notes/foo.md", + ) + assert len(findings) == 1 + cat, _line, _old, new = findings[0] + assert cat == "FIXABLE" + assert new == "/content/operate/rs/databases/active-active/develop/develop-for-aa.md" + + +if __name__ == "__main__": + import pytest + + sys.exit(pytest.main([__file__, "-v"])) From d2f6723e4d19280b2ee0859080e1247c95e24026 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Thu, 24 Sep 2026 10:45:02 +0100 Subject: [PATCH 2/2] DOC-7104: Fix MOUNT_PREFIX_RX to catch a bare ?query/#fragment with no slash The original regex required `/` or end-of-string right after the mount name, so a link like `/commands?group=cluster` (no trailing slash before the query) silently passed through unchecked -- this tool's own blind spot, found the hard way: human review caught it by hand on DOC-7104 PR #4093, and the identical instances recurred in #4094/#4096/#4098 before this fix existed to catch them. Also reconsiders the `/commands` special case: it has no backing _index.md on disk, so _find_content_file always reports it unresolvable, but Hugo auto-generates a section page for the directory and GetPage finds it anyway (confirmed by building both /commands?group=x and /content/commands?group=x and comparing rendered hrefs -- identical). Review wanted the canonical form applied there too, so it's now hardcoded as FIXABLE instead of silently skipped. Co-Authored-By: Claude Sonnet 5 --- build/check_uncanonicalized_links.py | 31 ++++++++++++++++------ build/test_check_uncanonicalized_links.py | 32 +++++++++++++++++++---- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/build/check_uncanonicalized_links.py b/build/check_uncanonicalized_links.py index 5bb338785a..51889b50ad 100644 --- a/build/check_uncanonicalized_links.py +++ b/build/check_uncanonicalized_links.py @@ -34,10 +34,16 @@ author concatenated onto it afterward. Reported only -- guessing the intended real target is a content-fact decision, not a mechanical fix. -`/commands` and `/commands/` are a fourth, silent case: the commands index -is templated with no backing `_index.md` (a known, accepted gap -- see -project memory reference_commands_group_link_convention), so a bare -`/commands/?group=x` link is correct AS WRITTEN and not a finding. +`/commands` is a special case handled separately from `_find_content_file`: +the commands index has no backing `_index.md` on disk (see project memory +reference_commands_group_link_convention), so the filesystem-based resolver +always reports it unresolvable. It isn't: Hugo auto-generates a section page +for any content directory even without an `_index.md`, and `GetPage` +finds it -- confirmed by building and comparing rendered hrefs for both +`/commands?group=x` and `/content/commands?group=x` (byte-identical). Human +review on DOC-7104 PR #4093 wanted the canonical `/content/` form applied +here too, so `/commands[...]` is hardcoded as FIXABLE to `/content/commands` +rather than silently skipped. Usage: build/check_uncanonicalized_links.py ... @@ -61,7 +67,13 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", ".claude", "hooks")) import check_shortcode_paths as csp # noqa: E402 -MOUNT_PREFIX_RX = re.compile(r'^/(operate|develop|integrate|commands)(/|$)') +# The lookahead admits `/`, `?`, `#`, or end-of-string right after the mount +# name -- not just `/` or end-of-string. `/commands?group=x` (no trailing +# slash before the query) is exactly the shape human review found missing +# its /content/ prefix on DOC-7104 PR #4093/#4094/#4096/#4098: the original +# `(/|$)` alternation required a slash or nothing, so a bare `?query` or +# `#fragment` right after the mount name silently passed through unchecked. +MOUNT_PREFIX_RX = re.compile(r'^/(operate|develop|integrate|commands)(?=[/?#]|$)') def find_root(start): @@ -94,10 +106,13 @@ def check_file(path, root): base_ref, suffix = (href[: split.start()], href[split.start() :]) if split else (href, "") bare = base_ref.rstrip("/") - if bare in ("/commands",): - continue # templated index, no backing file by design - line_no = text.count("\n", 0, m.start()) + 1 + if bare == "/commands": + # No _index.md on disk, but Hugo auto-generates a section page + # for the directory and GetPage finds it -- _find_content_file + # can't see that (filesystem-only), so it's hardcoded here. + findings.append(("FIXABLE", line_no, href, f"/content/commands{suffix}")) + continue target = msl._find_content_file(root, base_ref) if target is not None: findings.append(("FIXABLE", line_no, href, f"/{target}{suffix}")) diff --git a/build/test_check_uncanonicalized_links.py b/build/test_check_uncanonicalized_links.py index ae29013d10..1aaa9f18fd 100644 --- a/build/test_check_uncanonicalized_links.py +++ b/build/test_check_uncanonicalized_links.py @@ -98,16 +98,38 @@ def test_external_and_anchor_only_links_are_not_findings(): assert findings == [] -def test_bare_commands_index_with_group_query_is_not_a_finding(): - # /commands has no backing _index.md by design (templated index) -- - # this is the one accepted bare-path exception. +def test_bare_commands_index_is_fixable_to_content_prefix(): + # /commands has no backing _index.md on disk, so _find_content_file + # alone would call it DEAD -- hardcoded as FIXABLE instead, since Hugo's + # auto-generated section page resolves it identically either way + # (confirmed by building both forms during the DOC-7104 #4093 review). findings = run( { - "operate/rs/release-notes/foo.md": "See [commands](/commands/?group=cluster).", + "operate/rs/release-notes/foo.md": "See [commands](/commands?group=cluster).", }, "operate/rs/release-notes/foo.md", ) - assert findings == [] + assert len(findings) == 1 + cat, _line, old, new = findings[0] + assert cat == "FIXABLE" + assert old == "/commands?group=cluster" + assert new == "/content/commands?group=cluster" + + +def test_query_with_no_trailing_slash_before_it_is_still_caught(): + # Regression test: the original MOUNT_PREFIX_RX required `/` or + # end-of-string right after the mount name, so `/commands?group=x` (no + # slash before the `?`) silently passed through unchecked. This is the + # exact shape human review found bare on DOC-7104 PR #4093/#4094/#4096/#4098. + findings = run( + { + "operate/rs/security/access-control.md": "x", + "operate/rs/release-notes/foo.md": "[ACL](/operate/rs/security/access-control?x=1).", + }, + "operate/rs/release-notes/foo.md", + ) + assert len(findings) == 1 + assert findings[0][0] == "FIXABLE" def test_relref_plus_suffix_shape_matches_the_pr_4086_case():