mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 00:36:39 +00:00
fix(wiki): make a link target the article filename verbatim, not its percent-encoded twin (#2597)
The wiki exporter wrote each article as {slug}.md but emitted the link as
quote({slug}.md), so any label with ( ) & # or non-ASCII produced a
percent-encoded target that names no file on disk. Removes the quote() fork
and hardens _safe_filename so the slug is URL-safe by construction; the link
and the on-disk filename are now the same string.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
safishamsi
co-authored by
Claude Opus 4.8
parent
bd1ede9584
commit
fc00f673c6
+47
-13
@@ -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]]:
|
||||
|
||||
+30
-22
@@ -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()
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user