fix(wiki): make a link target the article filename verbatim, not its percent-encoded twin (#2597)

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]]:

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 @@
 
 
 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():
@@ -314,10 +320,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)
@@ -326,21 +335,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)
@@ -349,7 +357,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()
 
 

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('a<b>c: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