mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-14 16:09:26 +00:00
fix(extract): refuse a dedup shrink unless the user accepts it
A finished extract force-writes, so the node-count check never sees a
graph that dedup made smaller. Refuse that overwrite and name
--allow-dedup-shrink. A deleted file still writes.
Fixes #3774
Co-Authored-By: Grok 4.7 <noreply@x.ai>
(cherry picked from commit 686bef5855)
This commit is contained in:
1 parent
0110d962b9
commit
daee93f782
5 files changed
+222
-14
No files matched your search
+12
-1
@@ -1564,6 +1564,7 @@ def build(
|
||||
combined["input_tokens"] += ext.get("input_tokens", 0)
|
||||
combined["output_tokens"] += ext.get("output_tokens", 0)
|
||||
_root = str(Path(root).resolve()) if root else None
|
||||
_dedup_collapsed = 0
|
||||
if dedup and combined["nodes"]:
|
||||
# Numeric ids must be str before dedup, which keys on them and would
|
||||
# raise TypeError in _pick_winner's regex search (#2326). build_from_json
|
||||
@@ -1585,6 +1586,7 @@ def build(
|
||||
n["source_file"] = _norm_source_file(n["source_file"], _root)
|
||||
if "definition_file" in n:
|
||||
n["definition_file"] = _norm_source_file(n["definition_file"], _root)
|
||||
_before_dedup = len(combined["nodes"])
|
||||
combined["nodes"], combined["edges"] = deduplicate_entities(
|
||||
combined["nodes"], combined["edges"], communities={},
|
||||
dedup_llm_backend=dedup_llm_backend, root=_root,
|
||||
@@ -1593,7 +1595,13 @@ def build(
|
||||
hyperedges=combined.get("hyperedges"),
|
||||
protected_ids=protected_ids,
|
||||
)
|
||||
return build_from_json(combined, directed=directed, root=_root)
|
||||
_dedup_collapsed = _before_dedup - len(combined["nodes"])
|
||||
G = build_from_json(combined, directed=directed, root=_root)
|
||||
# CLI reads this to tell a dedup shrink from a file deletion (#3774).
|
||||
# Popped before to_json so it is not stored in graph.json.
|
||||
if _dedup_collapsed:
|
||||
G.graph["_dedup_collapsed"] = _dedup_collapsed
|
||||
return G
|
||||
|
||||
|
||||
def _norm_label(label: str | None) -> str:
|
||||
@@ -2249,6 +2257,9 @@ def build_merge(
|
||||
if orphaned:
|
||||
G.remove_nodes_from(orphaned)
|
||||
n_nodes += len(orphaned)
|
||||
# CLI uses this to excuse a smaller graph when the loss is a deletion (#3774).
|
||||
# Popped before to_json so it is not stored in graph.json.
|
||||
G.graph["_pruned_node_count"] = n_nodes
|
||||
|
||||
# Report only the prune entries that ACTUALLY matched something — not
|
||||
# len(prune_sources), which counted every entry as pruned-from even
|
||||
|
||||
+44
-9
@@ -3361,7 +3361,8 @@ def dispatch_command(cmd: str) -> None:
|
||||
"[--model M] [--mode deep] [--out DIR|--output DIR] [--google-workspace] [--no-cluster] "
|
||||
"[--no-gitignore] [--code-only] [--no-dedup] "
|
||||
"[--max-workers N] [--token-budget N] [--max-concurrency N] "
|
||||
"[--api-timeout S] [--postgres DSN] [--cargo] [--allow-partial] [--timing]",
|
||||
"[--api-timeout S] [--postgres DSN] [--cargo] [--allow-partial] "
|
||||
"[--allow-dedup-shrink] [--timing]",
|
||||
file=sys.stderr,
|
||||
)
|
||||
sys.exit(1)
|
||||
@@ -3383,6 +3384,9 @@ def dispatch_command(cmd: str) -> None:
|
||||
cli_postgres_dsn: str | None = None
|
||||
cli_cargo: bool = False
|
||||
cli_allow_partial: bool = False
|
||||
# --allow-dedup-shrink: write a graph that is smaller because dedup
|
||||
# merged nodes. A deleted file does not need this flag (#3774).
|
||||
cli_allow_dedup_shrink: bool = False
|
||||
no_cluster = False
|
||||
dedup_llm = False
|
||||
# --no-dedup: skip entity deduplication entirely. On an incremental
|
||||
@@ -3511,6 +3515,8 @@ def dispatch_command(cmd: str) -> None:
|
||||
force = True; i += 1
|
||||
elif a == "--allow-partial":
|
||||
cli_allow_partial = True; i += 1
|
||||
elif a == "--allow-dedup-shrink":
|
||||
cli_allow_dedup_shrink = True; i += 1
|
||||
elif a == "--timing":
|
||||
cli_timing = True; i += 1
|
||||
else:
|
||||
@@ -4676,15 +4682,17 @@ def dispatch_command(cmd: str) -> None:
|
||||
_backup(graphify_out)
|
||||
_invalidate_file_manifest_for_db_graph()
|
||||
# force=True bypasses the #479 shrink guard entirely. A full build
|
||||
# legitimately shrinks (fuzzy dedup collapse, deleted code) so it keeps
|
||||
# force=True — EXCEPT when this run's extraction was incomplete (an
|
||||
# extractor pass crashed or some semantic chunks failed). Then a partial
|
||||
# graph could silently overwrite a good complete one, so fall back to the
|
||||
# shrink guard (force=False) unless the user opts in with --allow-partial.
|
||||
# legitimately shrinks when files were deleted, so a complete run keeps
|
||||
# force=True — EXCEPT:
|
||||
# * extraction was incomplete (a pass crashed or a chunk came back
|
||||
# empty). Fall back to the shrink guard unless --allow-partial.
|
||||
# * dedup merged nodes and the saved graph would shrink by more than
|
||||
# this run's deletions (#3774). Refuse the write before to_json unless
|
||||
# --allow-dedup-shrink. --allow-partial does not accept that shrink.
|
||||
#
|
||||
# Both write paths are guarded: the clustered path here via to_json's
|
||||
# #479 check, and the `--no-cluster` raw-dump path above via the same
|
||||
# shrink check against the existing file (existing_graph_node_count).
|
||||
# Both write paths are guarded for an incomplete extraction: the clustered
|
||||
# path here via to_json's #479 check, and the `--no-cluster` raw-dump path
|
||||
# above via the same shrink check against the existing file.
|
||||
#
|
||||
# Trade-off: this reuses to_json's coarse node-count guard, not the
|
||||
# source-aware _check_shrink that watch/update use. On an incremental run
|
||||
@@ -4692,7 +4700,34 @@ def dispatch_command(cmd: str) -> None:
|
||||
# failure can therefore be refused here — recoverable by re-running or
|
||||
# passing --allow-partial (the good graph is preserved and the manifest
|
||||
# is not stamped, so the retry re-extracts).
|
||||
_gattrs = G.graph if hasattr(G, "graph") else {}
|
||||
_dedup_collapsed = int(_gattrs.pop("_dedup_collapsed", 0) or 0)
|
||||
_pruned_nodes = int(_gattrs.pop("_pruned_node_count", 0) or 0)
|
||||
_force_write = cli_allow_partial or not _extraction_incomplete
|
||||
_dedup_shrink_counts: tuple[int, int] | None = None
|
||||
if (
|
||||
not _extraction_incomplete
|
||||
and not no_dedup
|
||||
and not cli_allow_dedup_shrink
|
||||
and _dedup_collapsed > 0
|
||||
):
|
||||
from graphify.export import existing_graph_node_count as _existing_graph_node_count
|
||||
_existing_n = _existing_graph_node_count(graph_json_path)
|
||||
_new_n = G.number_of_nodes()
|
||||
if isinstance(_existing_n, int) and _new_n < _existing_n - _pruned_nodes:
|
||||
_dedup_shrink_counts = (_existing_n, _new_n)
|
||||
if _dedup_shrink_counts is not None:
|
||||
# Refuse before to_json. to_json's own refusal tells the caller to
|
||||
# pass force=True, which is not this flag (#3774).
|
||||
_old_n, _new_n = _dedup_shrink_counts
|
||||
print(
|
||||
"[graphify extract] error: dedup reduced the graph from "
|
||||
f"{_old_n} nodes to {_new_n} nodes. Refusing to overwrite "
|
||||
f"{graph_json_path}. Pass --allow-dedup-shrink to write "
|
||||
"the smaller graph.",
|
||||
file=sys.stderr,
|
||||
)
|
||||
sys.exit(1)
|
||||
# Stamp provenance from the ANALYSED repo, not the shell's cwd: without
|
||||
# this, to_json's fallback asks `git rev-parse HEAD` in whatever repo the
|
||||
# command was invoked from, so `graphify extract <target>` run from
|
||||
|
||||
@@ -0,0 +1,161 @@
|
||||
"""A complete `graphify extract` must not force-write a shrink that dedup caused (#3774).
|
||||
|
||||
A full build sets force=True, and to_json then skips the #479 node-count check.
|
||||
Dedup can merge a same-file duplicate and replace graph.json with no message.
|
||||
A deleted file is a different shrink: that write still happens.
|
||||
`--allow-dedup-shrink` is the only flag that accepts the dedup shrink.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
|
||||
import pytest
|
||||
|
||||
import graphify.__main__ as mainmod
|
||||
|
||||
|
||||
def _notes(corpus):
|
||||
readme = str(corpus / "README.md")
|
||||
return [
|
||||
{
|
||||
"id": "readme_decisions_a",
|
||||
"label": "Decisions",
|
||||
"file_type": "document",
|
||||
"source_file": readme,
|
||||
"source_location": "L1",
|
||||
},
|
||||
{
|
||||
"id": "readme_decisions_b",
|
||||
"label": "Decisions",
|
||||
"file_type": "document",
|
||||
"source_file": readme,
|
||||
"source_location": "L2",
|
||||
},
|
||||
{
|
||||
"id": "readme_overview",
|
||||
"label": "Overview",
|
||||
"file_type": "document",
|
||||
"source_file": readme,
|
||||
"source_location": "L3",
|
||||
},
|
||||
]
|
||||
|
||||
|
||||
def _write_graph(path, nodes):
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(
|
||||
json.dumps({"nodes": nodes, "links": []}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
|
||||
def _node_ids(path):
|
||||
data = json.loads(path.read_text(encoding="utf-8"))
|
||||
return [n["id"] for n in data["nodes"]]
|
||||
|
||||
|
||||
def _corpus(tmp_path):
|
||||
(tmp_path / "README.md").write_text("# Notes\nThe entry point overview.\n", encoding="utf-8")
|
||||
(tmp_path / "GUIDE.md").write_text("# Guide\nHow to use the thing.\n", encoding="utf-8")
|
||||
return tmp_path
|
||||
|
||||
|
||||
def _arm(monkeypatch, tmp_path, *, nodes, extra_argv=()):
|
||||
corpus = _corpus(tmp_path)
|
||||
out_dir = tmp_path / "out"
|
||||
graph_path = out_dir / "graphify-out" / "graph.json"
|
||||
_write_graph(graph_path, nodes)
|
||||
monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-test-fake-key")
|
||||
|
||||
def _stub_corpus(paths, **kwargs):
|
||||
on_chunk = kwargs.get("on_chunk_done")
|
||||
if on_chunk:
|
||||
on_chunk(0, 1, {"nodes": [], "edges": [], "hyperedges": []})
|
||||
return {
|
||||
"nodes": nodes,
|
||||
"edges": [],
|
||||
"hyperedges": [],
|
||||
"input_tokens": 10,
|
||||
"output_tokens": 5,
|
||||
}
|
||||
|
||||
monkeypatch.setattr("graphify.llm.extract_corpus_parallel", _stub_corpus)
|
||||
monkeypatch.setattr(mainmod, "_check_skill_version", lambda _: None)
|
||||
monkeypatch.setattr(
|
||||
mainmod.sys, "argv",
|
||||
["graphify", "extract", str(corpus), "--backend", "claude",
|
||||
"--out", str(out_dir), *extra_argv],
|
||||
)
|
||||
return graph_path
|
||||
|
||||
|
||||
def _run():
|
||||
try:
|
||||
mainmod.main()
|
||||
except SystemExit as exc:
|
||||
return exc.code
|
||||
return 0
|
||||
|
||||
|
||||
def test_complete_run_refuses_a_same_file_dedup_shrink(monkeypatch, tmp_path, capsys):
|
||||
"""Two same-file duplicates plus one keeper. Dedup merges the pair.
|
||||
The file on disk must keep all three nodes."""
|
||||
graph_path = _arm(monkeypatch, tmp_path, nodes=_notes(tmp_path))
|
||||
code = _run()
|
||||
err = capsys.readouterr().err
|
||||
assert code not in (None, 0)
|
||||
assert _node_ids(graph_path) == [
|
||||
"readme_decisions_a",
|
||||
"readme_decisions_b",
|
||||
"readme_overview",
|
||||
]
|
||||
assert "3" in err and "2" in err
|
||||
assert "--allow-dedup-shrink" in err
|
||||
|
||||
|
||||
def test_allow_dedup_shrink_writes_the_smaller_graph(monkeypatch, tmp_path):
|
||||
graph_path = _arm(
|
||||
monkeypatch, tmp_path, nodes=_notes(tmp_path),
|
||||
extra_argv=["--allow-dedup-shrink"],
|
||||
)
|
||||
code = _run()
|
||||
ids = _node_ids(graph_path)
|
||||
assert code in (None, 0)
|
||||
assert len(ids) == 2
|
||||
assert "readme_overview" in ids
|
||||
assert not {"readme_decisions_a", "readme_decisions_b"} <= set(ids)
|
||||
raw = graph_path.read_text(encoding="utf-8")
|
||||
assert "_dedup_collapsed" not in raw
|
||||
assert "_pruned_node_count" not in raw
|
||||
|
||||
|
||||
def test_a_deleted_file_still_writes(monkeypatch, tmp_path):
|
||||
"""The shrink is one missing file. Dedup merges nothing. The write proceeds."""
|
||||
corpus = tmp_path
|
||||
readme = str(corpus / "README.md")
|
||||
gone = str(corpus / "gone.md")
|
||||
nodes = [
|
||||
{
|
||||
"id": "readme_overview",
|
||||
"label": "Overview",
|
||||
"file_type": "document",
|
||||
"source_file": readme,
|
||||
"source_location": "L1",
|
||||
},
|
||||
{
|
||||
"id": "gone_notes",
|
||||
"label": "Gone notes",
|
||||
"file_type": "document",
|
||||
"source_file": gone,
|
||||
"source_location": "L1",
|
||||
},
|
||||
]
|
||||
fresh = [nodes[0]]
|
||||
graph_path = _arm(monkeypatch, tmp_path, nodes=fresh)
|
||||
# Replace the seeded graph with both files. The stub still returns only README.
|
||||
_write_graph(graph_path, nodes)
|
||||
code = _run()
|
||||
ids = _node_ids(graph_path)
|
||||
assert code in (None, 0)
|
||||
assert "gone_notes" not in ids
|
||||
assert "readme_overview" in ids
|
||||
@@ -97,8 +97,8 @@ def test_allow_partial_still_overrides(monkeypatch, tmp_path):
|
||||
|
||||
|
||||
def test_a_run_with_every_file_covered_keeps_force_write(monkeypatch, tmp_path, capsys):
|
||||
"""The ordinary complete run is unchanged: a full build legitimately
|
||||
shrinks (dedup, deleted code) and keeps bypassing the guard."""
|
||||
"""A complete run that did not shrink the graph via dedup still force-writes.
|
||||
A dedup shrink is refused unless --allow-dedup-shrink (#3774)."""
|
||||
rec = _record_force(monkeypatch)
|
||||
_arm(monkeypatch, tmp_path)
|
||||
_run()
|
||||
|
||||
@@ -100,8 +100,9 @@ def test_allow_partial_forces_write_despite_incomplete(monkeypatch, tmp_path):
|
||||
|
||||
|
||||
def test_complete_extraction_keeps_force_write(monkeypatch, tmp_path):
|
||||
# All chunks succeeded -> a complete build legitimately keeps force=True so a
|
||||
# genuine dedup/deletion shrink still overwrites.
|
||||
# All chunks succeeded and dedup did not shrink the saved graph. A complete
|
||||
# run still force-writes in that case. A dedup shrink is refused unless
|
||||
# --allow-dedup-shrink (#3774); see test_dedup_shrink_refuses_force_write.
|
||||
rec = _seed_to_json_recorder(monkeypatch, returns=True)
|
||||
_arm_extract(monkeypatch, tmp_path, chunk_total=1, chunk_succeeded=1)
|
||||
|
||||
|
||||
Reference in new issue
Block a user