diff --git a/graphify/wiki.py b/graphify/wiki.py index d283552a..d9032109 100644 --- a/graphify/wiki.py +++ b/graphify/wiki.py @@ -3,23 +3,47 @@ from __future__ import annotations from collections import Counter from pathlib import Path -from urllib.parse import quote +import re import networkx as nx from graphify.build import edge_data +# Characters a slug may not contain, because the article's LINK and its ON-DISK +# NAME have to be the same string (#2597). Anything left here must be legal, +# unescaped, in a CommonMark link destination: +# < > : " / \ | ? * Windows-reserved in filenames (pre-existing set) +# ( ) parentheses delimit/nest a link destination +# # starts a fragment, so `a#b.md` resolves to the file `a` +# % reads as the start of a percent-escape +# control chars forbidden in a link destination, hostile in a filename +# +# Non-ASCII is deliberately NOT stripped. It is legal raw in a link destination +# and resolves fine on every filesystem graphify targets; stripping it would +# reduce a CJK, Cyrillic or accented wiki to a wall of underscores. +_UNSAFE_SLUG_CHARS = re.compile(r'[<>:"/\\|?*#%\x00-\x1f\x7f]') + + def _safe_filename(name: str) -> str: - """Make a label safe for use as a filename across platforms. + """Make a label safe for use as a filename across platforms AND as a + markdown link destination. Substitutes characters that Windows reserves in filenames - (< > : " / \\ | ? *) and strips trailing dots/spaces, also reserved. + (< > : " / \\ | ? *) plus the ones that would make the emitted link stop + matching the file on disk, and strips trailing dots/spaces, also reserved. Falls back to 'unnamed' for empty results and caps length at 200 chars to stay well under common filesystem limits. + + Parentheses are DROPPED rather than substituted: every callable node is + labelled ``foo()``, and substituting would leave a trailing ``foo__`` on + each of them — and would mangle Python dunders (``__init__()`` -> + ``_init_``) if the resulting runs were then collapsed. Dropping keeps + ``__init__`` intact. Two labels that collapse to one slug are still + separated by ``_unique_slug``. """ - import re s = name.replace("/", "-").replace(" ", "_").replace(":", "-") - s = re.sub(r'[<>:"/\\|?*]', '_', s) + s = s.replace("(", "").replace(")", "") + s = _UNSAFE_SLUG_CHARS.sub('_', s) s = s.strip('. ') return s[:200] if s else 'unnamed' @@ -29,13 +53,23 @@ def _md_link(label: str, resolver: dict[str, str]) -> str: ``resolver`` maps an article's display label to the slug (filename stem) it was written under. When the label has an article, emit a standard - ``[label](slug.md)`` link, URL-encoding the target so any spaces, parens, & - or # in the slug survive every CommonMark renderer (GitHub, GitLab, VS Code - preview, a plain browser) and Obsidian alike. The old ``[[label]]`` form - only resolved inside Obsidian, because the on-disk filename differs from the - label — _safe_filename turns spaces into underscores and substitutes - reserved characters — so e.g. ``[[Domain Data Models]]`` pointed at a - non-existent ``Domain Data Models.md`` everywhere else. + ``[label](slug.md)`` link whose target is the on-disk name VERBATIM. + + The target is deliberately not percent-encoded (#2597). ``quote()`` turned + ``_make_id().md`` into ``_make_id%28%29.md`` while the file stayed raw, so + the link pointed at a path that does not exist. Renderers hid it by + decoding before resolving, but the wiki's whole purpose is to be + agent-crawlable, and an agent that reads the target off disk verbatim got a + FileNotFoundError. ``_safe_filename`` now keeps the slug free of everything + that would need encoding, so raw emission and the filename are the same + string by construction — one source of truth instead of two spellings that + happened to agree only for URL-safe labels. + + The old ``[[label]]`` form only resolved inside Obsidian, because the + on-disk filename differs from the label — _safe_filename turns spaces into + underscores and substitutes reserved characters — so e.g. + ``[[Domain Data Models]]`` pointed at a non-existent + ``Domain Data Models.md`` everywhere else. Labels with no article — most node-level links, since only communities and god nodes get article files — render as plain text instead of a dead link @@ -45,7 +79,7 @@ def _md_link(label: str, resolver: dict[str, str]) -> str: slug = resolver.get(label) if slug is None: return text - return f"[{text}]({quote(f'{slug}.md')})" + return f"[{text}]({slug}.md)" def _cross_community_links(G: nx.Graph, nodes: list[str], own_cid: int, labels: dict[int, str], node_community: dict[str, int]) -> list[tuple[str, int]]: diff --git a/tests/test_wiki.py b/tests/test_wiki.py index 18013da6..130da843 100644 --- a/tests/test_wiki.py +++ b/tests/test_wiki.py @@ -1,6 +1,5 @@ """Tests for graphify.wiki — Wikipedia-style article generation.""" import re -import urllib.parse import pytest from pathlib import Path import networkx as nx @@ -10,14 +9,21 @@ _MD_LINK = re.compile(r"\[([^\]]+)\]\(([^)]+)\)") def _inline_links(text): - """Yield (display, decoded_target) for each inline markdown link, skipping - external URLs. Targets are URL-decoded so they can be checked against the - on-disk filename. (Display text with an escaped `]` isn't matched, but the - generated labels used in link position never contain brackets.)""" + """Yield (display, target) for each inline markdown link, skipping external + URLs. The target is yielded VERBATIM. + + This used to `urllib.parse.unquote` it, which is why + `test_wiki_links_resolve_to_real_files` passed all the way through #2597: + the emitted target was `_make_id%28%29.md` while the file on disk was + `_make_id().md`, and decoding in the helper papered over exactly the + mismatch the guard exists to catch. The link and the filename must be the + same string, so the check has to compare them as written. + (Display text with an escaped `]` isn't matched, but the generated labels + used in link position never contain brackets.)""" for display, target in _MD_LINK.findall(text): if "://" in target: continue - yield display, urllib.parse.unquote(target) + yield display, target def _make_graph(): @@ -345,10 +351,13 @@ def test_wiki_link_display_keeps_label_but_target_is_filename(tmp_path): def test_wiki_special_characters_in_label_resolve(tmp_path): - """Labels with spaces, &, #, and parentheses must still produce a link whose - URL-encoded target decodes back to the real (underscored) filename, so it - works in CommonMark renderers and Obsidian alike. # is the dangerous one — - left raw in a relative link it would be misread as a fragment.""" + """Labels with spaces, &, #, and parentheses must produce a link whose target + IS the on-disk filename, with no encoding step in between (#2597). + + `#` and `(` `)` are the dangerous ones: left raw in a relative link, `#` + would be misread as a fragment and `)` would terminate the destination + early. They are therefore kept out of the slug entirely rather than encoded + into a target that no longer names the file.""" G = nx.Graph() G.add_node("n1", label="a", file_type="code", source_file="a.py", community=0) G.add_node("n2", label="b", file_type="code", source_file="b.py", community=1) @@ -357,21 +366,20 @@ def test_wiki_special_characters_in_label_resolve(tmp_path): labels = {0: "C# & Auth (v2)", 1: "Other"} to_wiki(G, communities, tmp_path, community_labels=labels) article = (tmp_path / "Other.md").read_text() - # the cross-link to the special-char community resolves to its real file targets = [t for _, t in _inline_links(article)] - assert "C#_&_Auth_(v2).md" in targets - assert (tmp_path / "C#_&_Auth_(v2).md").exists() - # the raw target is fully percent-encoded — no bare ( ) that would terminate - # the link early, no bare # that would be misread as a fragment - assert "C%23_%26_Auth_%28v2%29.md" in article + assert "C__&_Auth_v2.md" in targets + assert (tmp_path / "C__&_Auth_v2.md").exists() + # no percent-escape in any target: it is the filename verbatim + assert not any("%" in t for t in targets), targets + # & survives — it is legal raw in a link destination and in a filename + assert any("&" in t for t in targets), targets def test_wiki_link_with_bracketed_label_resolves(tmp_path): - """A label containing `[` / `]` (e.g. a generic like `Array[T]`) still - produces a resolvable link: the brackets are escaped in the display text so - they don't break the markdown, and percent-encoded in the target so it - decodes back to the real file. (`_safe_filename` keeps brackets in the slug, - so they reach the link target.)""" + """A label containing `[` / `]` (e.g. a generic like `Array[T]`) produces a + resolvable link: the brackets are escaped in the display text so they don't + break the markdown, and kept verbatim in the target, which is legal in a + CommonMark link destination and matches the file on disk (#2597).""" G = nx.Graph() G.add_node("n1", label="a", file_type="code", source_file="a.py", community=0) G.add_node("n2", label="b", file_type="code", source_file="b.py", community=1) @@ -380,7 +388,7 @@ def test_wiki_link_with_bracketed_label_resolves(tmp_path): labels = {0: "Array[T] Models", 1: "Other"} to_wiki(G, communities, tmp_path, community_labels=labels) article = (tmp_path / "Other.md").read_text() - assert r"[Array\[T\] Models](Array%5BT%5D_Models.md)" in article + assert r"[Array\[T\] Models](Array[T]_Models.md)" in article assert (tmp_path / "Array[T]_Models.md").exists() diff --git a/tests/test_wiki_link_filename_parity.py b/tests/test_wiki_link_filename_parity.py new file mode 100644 index 00000000..69a9a224 --- /dev/null +++ b/tests/test_wiki_link_filename_parity.py @@ -0,0 +1,150 @@ +"""Regression tests for issue #2597: a wiki link's target must BE the on-disk +filename, byte for byte. + +`_md_link` percent-encoded the slug (`_make_id%28%29.md`) while `to_wiki` wrote +the file raw (`_make_id().md`), so every article whose label contained `(`, `)`, +`&` or a non-ASCII character was linked at a path that does not exist. Renderers +hid it by decoding before resolving, but the wiki's stated purpose is to be +agent-crawlable, and an agent that reads the target off disk verbatim gets a +FileNotFoundError. Function-named nodes (`foo()`) make this common in any code +repo — 27 of 1141 links on a 2247-node graph. + +The invariant these tests pin down: for every inline link the wiki emits, +`(wiki_dir / target).exists()` is true WITHOUT any unquoting step. +""" +import re + +import networkx as nx +import pytest + +from graphify.wiki import _safe_filename, to_wiki + +# Deliberately does not decode: the target is compared exactly as written. +_MD_LINK = re.compile(r"\[([^\]]+)\]\(([^)]+)\)") + + +def _targets(text: str) -> list[str]: + return [t for _d, t in _MD_LINK.findall(text) if "://" not in t] + + +def _wiki(tmp_path, labels: dict[int, str], god: list[dict] | None = None): + G = nx.Graph() + communities: dict[int, list[str]] = {} + for cid in labels: + nid = f"n{cid}" + G.add_node(nid, label=f"sym{cid}", file_type="code", + source_file=f"m{cid}.py", community=cid) + communities[cid] = [nid] + ids = list(G.nodes) + for a, b in zip(ids, ids[1:]): + G.add_edge(a, b, relation="references", confidence="INFERRED", weight=1.0) + out = tmp_path / "wiki" + to_wiki(G, communities, out, community_labels=labels, god_nodes_data=god or []) + return out + + +def _assert_every_link_resolves(out) -> int: + seen = 0 + for md in out.glob("*.md"): + for target in _targets(md.read_text(encoding="utf-8")): + seen += 1 + assert (out / target).exists(), ( + f"{md.name}: link target {target!r} does not exist on disk" + ) + assert seen, "expected the wiki to emit inline links" + return seen + + +# --------------------------------------------------------------------------- +# The character classes from the report +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize( + "label", + [ + "load_traumas()", # the common case: any callable + "__init__()", # dunder must survive paren removal + "Forgejo upgrade & rollback (runbook)", # & plus parens + "Tailscale HTTPS endpoints — how services get URLs", # em dash (non-ASCII) + "C# & Auth (v2)", # # would be read as a fragment + "100% coverage", # % would read as a percent-escape + "文档 索引", # CJK must not be reduced to noise + "Array[T] Models", # brackets are legal in a destination + ], +) +def test_link_target_is_the_filename_verbatim(tmp_path, label): + out = _wiki(tmp_path, {0: label, 1: "Other"}) + _assert_every_link_resolves(out) + + +def test_no_link_target_is_percent_encoded(tmp_path): + out = _wiki(tmp_path, {0: "Forgejo upgrade & rollback (runbook)", 1: "Other"}) + for md in out.glob("*.md"): + for target in _targets(md.read_text(encoding="utf-8")): + assert "%" not in target, f"{md.name}: target still encoded: {target}" + + +# --------------------------------------------------------------------------- +# _safe_filename's own guarantees +# --------------------------------------------------------------------------- + +def test_slug_drops_parens_without_mangling_dunders(): + # Substituting "(" / ")" with "_" would leave "__init______"; collapsing the + # runs afterwards would corrupt the dunder to "_init_". Dropping does neither. + assert _safe_filename("__init__()") == "__init__" + assert _safe_filename("load_traumas()") == "load_traumas" + + +def test_slug_has_nothing_that_needs_url_encoding(): + from urllib.parse import quote + for label in [ + "load_traumas()", "C# & Auth (v2)", "100% coverage", + "Forgejo upgrade & rollback (runbook)", "a/b:c*d?e", 'q"uote', "ctrl\x07char", + ]: + slug = _safe_filename(label) + # `&` and friends are legal raw in a link destination; the ones that are + # NOT must be gone, so quoting with them marked safe is a no-op. + assert quote(slug, safe="&+,;=@$!'~[]") == slug, (label, slug) + + +def test_slug_keeps_non_ascii(): + # Stripping non-ASCII would reduce a CJK or Cyrillic wiki to underscores. + assert _safe_filename("文档 索引") == "文档_索引" + assert _safe_filename("Ünicode Straße") == "Ünicode_Straße" + + +def test_slug_still_strips_windows_reserved_characters(): + slug = _safe_filename('ac:d"e/f\\g|h?i*j') + for ch in '<>:"/\\|?*': + assert ch not in slug, (ch, slug) + + +def test_distinct_labels_collapsing_to_one_slug_stay_distinct(tmp_path): + # "parse()" and "parse" both slug to "parse"; _unique_slug must separate them. + out = _wiki(tmp_path, {0: "parse()", 1: "parse", 2: "Other"}) + _assert_every_link_resolves(out) + names = sorted(p.name for p in out.glob("*.md")) + assert len(names) == len(set(names)) + assert "parse.md" in names and "parse_2.md" in names, names + + +# --------------------------------------------------------------------------- +# The whole-wiki guard, on a graph shaped like a real code repo +# --------------------------------------------------------------------------- + +def test_whole_wiki_has_no_dangling_link_with_callable_god_nodes(tmp_path): + G = nx.Graph() + for i, lab in enumerate(["_make_id()", "_read_text()", "__init__()", "Path"]): + G.add_node(f"g{i}", label=lab, file_type="code", + source_file=f"src/m{i}.py", community=i % 2) + ids = list(G.nodes) + for a in ids: + for b in ids: + if a != b: + G.add_edge(a, b, relation="calls", confidence="EXTRACTED", weight=1.0) + god = [{"id": n, "label": G.nodes[n]["label"], "degree": G.degree(n)} for n in ids] + out = tmp_path / "wiki" + to_wiki(G, {0: ["g0", "g2"], 1: ["g1", "g3"]}, out, + community_labels={0: "Ident & IDs (core)", 1: "I/O — helpers"}, + god_nodes_data=god) + assert _assert_every_link_resolves(out) > 5