mirror of
https://github.com/safishamsi/graphify.git
synced 2026-09-26 07:25:50 +00:00
fix: surface silently-skipped dirs in enumeration + dedup Pascal edges
Two correctness fixes found while analysing the reported 'graphify update occasionally writes a partial graph.json' bug. Enumeration (P0): detect()'s os.walk had no onerror handler, so any os.scandir failure -- a transient PermissionError, or a directory created/deleted mid-walk by concurrent writes (e.g. benchmarking racing the scan) -- was silently swallowed and that entire subtree dropped out of the file list with no log, no error. Downstream that becomes a silently partial graph.json. The walk now records each skipped directory (surfaced as walk_errors in detect()'s result) and warns to stderr, while still enumerating the rest of the tree. This stays visible even when a --force/GRAPHIFY_FORCE rebuild bypasses the shrink guards. Relatedly, to_json's #479 anti-shrink guard was fail-OPEN: a non-empty but unreadable existing graph.json (corrupt or mid-write) proceeded with the overwrite. It now fails SAFE -- refuse and point at force=True -- while an empty/whitespace existing file (no nodes to lose) still proceeds. The size-cap check keeps running before any read, so an oversized existing file is not loaded into memory. Pascal edges (P1): a class method declared in the interface section and defined in the implementation section each emitted a "method" edge to the same node id, and the edge helpers (unlike the node helpers) did not dedup, so ~half of a Pascal/Delphi graph's method edges were doubled -- inflating degree/centrality and tripping the #1739 cross-file resolver's single-owner god-node guard. Both extractors now dedup edges on (source, target, relation). Adds regression tests for all three behaviours. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
d89efbf9ef
commit
d2d1f68ff9
@@ -1597,3 +1597,38 @@ def test_detect_unclassified_empty_when_all_supported(tmp_path):
|
||||
(tmp_path / "README.md").write_text("# hi\n")
|
||||
res = detect(tmp_path)
|
||||
assert res.get("unclassified", []) == []
|
||||
|
||||
|
||||
def test_detect_reports_walk_errors_key():
|
||||
"""detect() always surfaces a walk_errors list so callers can tell whether
|
||||
enumeration was complete."""
|
||||
import tempfile
|
||||
d = Path(tempfile.mkdtemp())
|
||||
(d / "a.py").write_text("def f(): pass\n")
|
||||
res = detect(d)
|
||||
assert "walk_errors" in res
|
||||
assert res["walk_errors"] == []
|
||||
|
||||
|
||||
def test_detect_surfaces_unreadable_dir_instead_of_silent_skip(tmp_path, capsys):
|
||||
"""os.walk silently skips a subtree whose scandir raises (permissions, or a
|
||||
dir deleted mid-walk); that under-enumeration used to be invisible and could
|
||||
yield a silently partial graph. detect() now records it in walk_errors and
|
||||
warns, while still enumerating the rest of the tree."""
|
||||
import os
|
||||
if os.geteuid() == 0:
|
||||
import pytest
|
||||
pytest.skip("running as root: chmod 000 does not block scandir")
|
||||
(tmp_path / "a.py").write_text("def f(): pass\n")
|
||||
locked = tmp_path / "locked"
|
||||
locked.mkdir()
|
||||
(locked / "b.py").write_text("def g(): pass\n")
|
||||
os.chmod(locked, 0o000)
|
||||
try:
|
||||
res = detect(tmp_path)
|
||||
finally:
|
||||
os.chmod(locked, 0o755) # restore for cleanup
|
||||
code = res["files"]["code"]
|
||||
assert any(f.endswith("a.py") for f in code) # rest of tree still enumerated
|
||||
assert len(res["walk_errors"]) >= 1
|
||||
assert "could not scan" in capsys.readouterr().err
|
||||
|
||||
@@ -603,3 +603,39 @@ def test_backup_env_disable(tmp_path, monkeypatch):
|
||||
(tmp_path / "graph.json").write_text('{"nodes":[],"links":[]}')
|
||||
(tmp_path / ".graphify_semantic_marker").write_text("{}")
|
||||
assert backup_if_protected(tmp_path) is None
|
||||
|
||||
|
||||
def _mkG(n):
|
||||
import networkx as nx
|
||||
G = nx.Graph()
|
||||
for i in range(n):
|
||||
G.add_node(f"n{i}", label=f"n{i}", community=0)
|
||||
return G
|
||||
|
||||
|
||||
def test_to_json_refuses_shrink(tmp_path):
|
||||
"""#479: refuse to silently overwrite an existing graph with fewer nodes."""
|
||||
p = tmp_path / "graph.json"
|
||||
json.dump({"nodes": [{"id": f"n{i}"} for i in range(5)]}, p.open("w"))
|
||||
assert to_json(_mkG(2), {}, str(p), force=False) is False
|
||||
assert to_json(_mkG(2), {}, str(p), force=True) is True # force overrides
|
||||
|
||||
|
||||
def test_to_json_fails_safe_on_corrupt_existing(tmp_path):
|
||||
"""A non-empty but unparseable existing graph.json (corrupt or mid-write)
|
||||
must NOT be silently overwritten — we can't verify the new graph isn't a
|
||||
partial shrink, so fail safe (refuse) unless force is given."""
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text("{ this has content but is not valid json")
|
||||
assert to_json(_mkG(10), {}, str(p), force=False) is False
|
||||
assert to_json(_mkG(10), {}, str(p), force=True) is True
|
||||
|
||||
|
||||
def test_to_json_proceeds_on_empty_existing(tmp_path):
|
||||
"""An empty/whitespace existing file has no nodes to lose, so it is not a
|
||||
shrink risk — the write proceeds."""
|
||||
p = tmp_path / "graph.json"
|
||||
p.write_text("")
|
||||
assert to_json(_mkG(3), {}, str(p), force=False) is True
|
||||
data = json.loads(p.read_text())
|
||||
assert len(data["nodes"]) == 3
|
||||
|
||||
@@ -320,3 +320,26 @@ def test_dfm_dispatch_registered():
|
||||
def test_dfm_detect_extension_registered():
|
||||
from graphify.detect import CODE_EXTENSIONS
|
||||
assert ".dfm" in CODE_EXTENSIONS
|
||||
|
||||
|
||||
def _dup_edges(r):
|
||||
from collections import Counter
|
||||
triples = Counter((e["source"], e["target"], e["relation"]) for e in r["edges"])
|
||||
return {k: v for k, v in triples.items() if v > 1}
|
||||
|
||||
|
||||
def test_pascal_no_duplicate_method_edges_tree_sitter():
|
||||
"""A class method appears in both the interface declaration and the
|
||||
implementation; each used to emit a `method` edge to the same node, so the
|
||||
graph carried doubled method/contains/inherits edges (skewing degree and
|
||||
breaking the cross-file inherited-call resolver's god-node guard). Edges are
|
||||
now deduped on (source, target, relation)."""
|
||||
from graphify.extract import extract_pascal
|
||||
r = extract_pascal(FIXTURES / "sample.pas")
|
||||
assert _dup_edges(r) == {}, f"duplicate edges: {_dup_edges(r)}"
|
||||
|
||||
|
||||
def test_pascal_no_duplicate_method_edges_regex():
|
||||
from graphify.extract import _extract_pascal_regex
|
||||
r = _extract_pascal_regex(FIXTURES / "sample.pas")
|
||||
assert _dup_edges(r) == {}, f"duplicate edges: {_dup_edges(r)}"
|
||||
|
||||
Reference in New Issue
Block a user