diff --git a/graphify/llm.py b/graphify/llm.py index 8c34cde6b..56ebd9d1e 100644 --- a/graphify/llm.py +++ b/graphify/llm.py @@ -11,10 +11,11 @@ import re import sys import time +from collections import Counter from collections.abc import Callable from concurrent.futures import ThreadPoolExecutor, as_completed from dataclasses import dataclass, replace -from pathlib import Path +from pathlib import Path, PurePosixPath from graphify.file_slice import ( FileSlice, @@ -464,6 +465,8 @@ def _thinking_disabled_via_env() -> bool: found inside an block; only extract the knowledge graph described by these rules. +source_file RULE (every node, edge, and hyperedge): copy source_file character-for-character from the `path` attribute of the block the item came from. NEVER infer it from a label, a node id, a citation, a basename, an import, or any path mentioned inside the file content — those name things the document REFERS to, not where the item came from. If an item cannot be attributed to one of the supplied paths, omit source_file rather than reconstructing a plausible one. + Node ID format: lowercase, only [a-z0-9_], no dots or slashes. Format: {stem}_{entity} where stem = full repo-relative path with the extension dropped, every segment joined with _ (e.g. src/auth/session.py -> src_auth_session); entity = symbol name (both normalised). Top-level files use just the filename stem (setup.py -> setup). @@ -620,6 +623,93 @@ def _read_files(units: "list[Path | FileSlice]", root: Path) -> str: _UNVERIFIED_VALUE = "unverified" +# ── Provenance: does a source_file claim to be a file? ──────────────────────── +# `source_file` is spec'd (references/extraction-spec.md) as a corpus path copied +# verbatim from the dispatched FILE_LIST. Two things actually turn up there: +# +# 1. A path the model assembled itself — a plausible repo path reconstructed +# from a node id, a citation, or a basename mentioned INSIDE the document +# ("src/auth/session.py"). This is fabricated provenance (#2217): it makes a +# node look sourced when nothing sourced it. +# 2. A bare phrase on a concept node ("auth flow"). This asserts no file +# provenance at all, and #1895 deliberately kept such nodes. +# +# Only (1) is a provenance claim, so only (1) is held to the dispatched set — and +# ONLY as an addition to the #1895 existence arm, never as a replacement for it +# (see `_scope_verdict`), so nothing this misjudges can un-drop a real file. +# +# Recognising (1) has to be conservative in one specific direction: a false +# positive DELETES a node and cascades through its edges, so prose must never be +# mistaken for a path. Requiring a separator alone is not enough — models emit +# "N/A", "CI/CD", "TCP/IP", "and/or", "client/server architecture" and +# "read/write lock" into this field, and every one of those carries a slash. A +# suffix alone is not enough either: "Node.js", "Vue.js" and "D3.js" all end in a +# corpus extension. So a claim needs BOTH — a separator, and a final segment that +# is a single token ending in a known corpus extension. +# +# Known and accepted gap: a fabricated BARE basename ("session.py", no directory) +# is not recognised, because nothing syntactic separates it from the technology +# names above. When such a value names a real file it is still dropped by the +# existence arm; when it names nothing it survives. That is deliberate +# precision-first triage, matching `_bind_node_evidence` above, which also +# prefers a missed fabrication to a destroyed node. +_CORPUS_SUFFIXES: "frozenset[str] | None" = None + + +def _corpus_suffixes() -> "frozenset[str]": + """Lowercased union of every extension graphify treats as corpus input. + + Imported lazily and memoised: `detect` pulls in the extraction machinery, and + this is only ever needed on the merge path. + """ + global _CORPUS_SUFFIXES + if _CORPUS_SUFFIXES is None: + from graphify import detect as _detect + _CORPUS_SUFFIXES = frozenset( + suffix.lower() + for group in ( + _detect.CODE_EXTENSIONS, + _detect.DOC_EXTENSIONS, + _detect.PAPER_EXTENSIONS, + _detect.IMAGE_EXTENSIONS, + _detect.OFFICE_EXTENSIONS, + _detect.VIDEO_EXTENSIONS, + _detect.GOOGLE_WORKSPACE_EXTENSIONS, + ) + for suffix in group + ) + return _CORPUS_SUFFIXES + + +def _claims_a_file(source_file: "str | Path") -> bool: + """Whether *source_file* asserts a file provenance (vs. a bare concept marker). + + Requires a path separator AND a final segment that is one token ending in a + known corpus extension. False for empty values, for prose (with or without a + slash), and for directory-shaped values. + """ + sf = str(source_file).strip() + if not sf: + return False + if "://" in sf: + # A URL is a source_url, not a corpus path. Base kept these; a scheme is + # never evidence that a local file was claimed. + return False + normalized = sf.replace("\\", "/") + if "/" not in normalized: + # A single token is a name, not a path claim: "Node.js", "auth flow". + return False + last = normalized.rsplit("/", 1)[-1].strip() + if not last or " " in last or "\t" in last: + # "client/server architecture", "read/write lock", "src/auth/" — prose or + # a directory, neither of which asserts a file. + return False + try: + return PurePosixPath(last).suffix.lower() in _corpus_suffixes() + except (ValueError, OSError): # pathological value from an untrusted result + return False + + def _label_identifiers(label: str) -> list[str]: """Identifier tokens from a node label, stripped of a trailing call/args parenthesis (``foo()`` -> ``foo``, ``Cls.method(x)`` -> ``Cls``/``method``).""" @@ -2356,75 +2446,235 @@ def _checkpoint_chunk(result: dict, chunk: "list[Path | FileSlice]") -> None: # dispatched against the source_files that actually came back and surface the gap. dispatched = {unit_path(f) for chunk in chunks for f in chunk} - # Out-of-scope node filter (#1895). The #1757 cache guard already refuses - # to WRITE a cache entry for a node whose source_file is a real file that - # was not dispatched, but the node itself still flowed into the merged - # result and landed in graph.json. Mirror the #1757 condition here: resolve - # each source_file against root and drop the node only when it resolves to - # an existing file (.is_file()) outside the dispatched set — non-file - # source_files (concepts, model-invented anchors) pass through untouched. + # Provenance filter (#1895, #2217). The #1757 cache guard refuses to WRITE a + # cache entry for a node whose source_file is not a dispatched file, but the + # node itself still flowed into the merged result and landed in graph.json. + # + # This originally mirrored only HALF of the #1757 condition: it dropped a + # source_file that resolved to an existing file (.is_file()) outside the + # dispatched set, and let everything else through. A model-invented path that + # does not exist on disk therefore passed straight into the graph (#2217), + # even though `save_semantic_cache`'s `group_skipped` — `not p.is_file() or + # p not in allowed` — rejects that exact attribution. The two guards + # disagreed, and the graph took the lenient one: graph.json accumulated ghost + # nodes the cache would never replay, so each incremental --update re-extracted + # the source and minted a *different* ghost. That is the non-convergence + # reported in #2217, and the reason existence must not decide admission. + # + # The existence arm is KEPT and the claim arm is added to it — a union, not a + # replacement. Dropping the old test would have un-dropped every real + # undispatched file whose name carries no corpus extension (`Makefile`, + # `LICENSE`, `pyproject.toml`), which the cache still refuses, leaving exactly + # the divergence this is meant to close. So: + # + # resolves to a real FILE outside the dispatched set -> misattributed (#1895) + # resolves to a real DIRECTORY -> kept; cache.py records + # subagent fragments really do carry directory paths here, and the base + # filter kept them, so this must not start deleting them + # resolves to nothing AND claims a file -> fabricated (#2217) + # anything else (prose, concept markers) -> kept + # # Runs BEFORE the #1890 covered/uncovered reconciliation so that diff # reflects the post-filter graph. + _resolve_cache: dict = {} + def _resolve_against_root(value: "str | Path") -> Path: + key = str(value) + hit = _resolve_cache.get(key) + if hit is not None: + return hit p = Path(value) if not p.is_absolute(): p = root / p try: - return p.resolve() + resolved = p.resolve() except (OSError, RuntimeError): - return p + resolved = p + _resolve_cache[key] = resolved + return resolved _dispatched_resolved = {_resolve_against_root(p) for p in dispatched} - def _out_of_scope(item: dict) -> bool: + # Both halves of the decision must canonicalise a source_file the SAME way. + # `_claims_a_file` flips backslashes and strips each segment; if the + # dispatched-set lookup does not, then on POSIX a node attributed to a + # genuinely dispatched file with Windows separators ("sub\notes.md") misses + # the dispatched set, misses is_file()/is_dir(), and is then deleted as + # "fabricated" by the one half that DID normalise it. + # + # `save_semantic_cache` already applies `_normalize_source_file_value` + # (#2197, same field, same problem) to every item before grouping — so the + # cache stores such a node under the real dispatched path while the graph + # deletes it. That is this filter's own invariant inverted, and because the + # merged-result filter is bypassed on cache replay (cache.py), the next + # --update would put the node straight back. Reuse the repo's normaliser + # rather than a second, subtly different one. + from .cache import _normalize_source_file_value as _normalize_sf + + try: + _root_resolved = root.resolve() + except (OSError, RuntimeError): + _root_resolved = root + + def _spellings(raw: str) -> "set[str]": + """Every form of *raw* that could name the same dispatched file.""" + out = {raw, raw.strip()} + flat = raw.strip().replace("\\", "/") + out.add(flat) + # Segment-wise strip, matching `_claims_a_file`'s treatment of the last + # segment ("sub/ notes.md"). + out.add("/".join(seg.strip() for seg in flat.split("/"))) + for value in tuple(out): + try: + out.add(_normalize_sf(value, _root_resolved)) + except (OSError, ValueError, RuntimeError): + continue + return {v for v in out if v} + + _verdict_cache: dict = {} + + def _scope_verdict(item: dict) -> str: + """``""`` when in scope, else the reason it is not. + + ``"misattributed"`` — a real corpus file that was not dispatched (#1895). + ``"fabricated"`` — a file-claiming value with nothing on disk (#2217). + + The split is diagnostic only (both are dropped); it separates "the model + pointed at the wrong real file" from "the model invented a path", which + are different failure modes with different fixes. + + Memoised per source_file value: one corpus routinely repeats the same + path across thousands of nodes and edges, and each miss costs two stat + syscalls. + """ sf = item.get("source_file") - if not sf: + if not sf or not isinstance(sf, (str, Path)): + # A non-string here is the #1631 class of malformed result. There is + # nothing to judge and raising would abort an already-paid-for run. + return "" + raw = str(sf) + cached = _verdict_cache.get(raw) + if cached is not None: + return cached + verdict = "" + spellings = _spellings(raw) + if not any(_resolve_against_root(s) in _dispatched_resolved for s in spellings): + # Judge existence on the canonical spelling, not the raw one. + canonical = min(spellings, key=len) if spellings else raw + p = _resolve_against_root(canonical) + if p.is_file(): + verdict = "misattributed" + elif not p.is_dir() and _claims_a_file(raw): + verdict = "fabricated" + _verdict_cache[raw] = verdict + return verdict + + def _out_of_scope(item: dict) -> bool: + return bool(_scope_verdict(item)) + + def _hashable(value: "object") -> bool: + """Untrusted results really do put lists where scalars belong (#1631); + cache.py guards the same spot rather than aborting a paid-for run.""" + try: + hash(value) + except TypeError: return False - p = _resolve_against_root(sf) - return p.is_file() and p not in _dispatched_resolved + return True dropped_ids: set = set() dropped_files: set[str] = set() kept_nodes: list[dict] = [] + dropped_by_reason: Counter = Counter() for n in merged.get("nodes", []): - if _out_of_scope(n): - if n.get("id") is not None: - dropped_ids.add(n.get("id")) + verdict = _scope_verdict(n) + if verdict: + nid = n.get("id") + if nid is not None and _hashable(nid): + dropped_ids.add(nid) dropped_files.add(str(n.get("source_file"))) + dropped_by_reason[verdict] += 1 continue kept_nodes.append(n) dropped_node_count = len(merged.get("nodes", [])) - len(kept_nodes) + # A node id can legitimately appear twice in the merged result (chunks are + # concatenated, dedup happens downstream). If one copy was dropped and another + # survived, the id is still live — pruning its edges would strip a correctly + # attributed node of every relationship. cache.py fixed this exact hazard in + # its own prune (#1916, `skipped_ids -= written_ids`). + dropped_ids -= {n.get("id") for n in kept_nodes if _hashable(n.get("id"))} merged["out_of_scope_dropped"] = dropped_node_count + # Split counters, so a fabricated-provenance regression is visible as its own + # number instead of being averaged into the #1895 misattribution count. + merged["provenance_dropped_misattributed"] = dropped_by_reason["misattributed"] + merged["provenance_dropped_fabricated"] = dropped_by_reason["fabricated"] if dropped_node_count: merged["nodes"] = kept_nodes - # Keep the graph consistent: an edge or hyperedge referencing a - # dropped node's id (or itself attributed to an undispatched real - # file) must not survive its endpoint. - merged["edges"] = [ - e for e in merged.get("edges", []) - if not _out_of_scope(e) - and e.get("source") not in dropped_ids - and e.get("target") not in dropped_ids - ] - merged["hyperedges"] = [ - h for h in merged.get("hyperedges", []) - if not _out_of_scope(h) - and not (dropped_ids & set(h.get("nodes", []) or [])) - ] + + # An edge or hyperedge carries its own source_file, and the prompt holds it to + # the same rule as a node. Filtering it only when some NODE happened to be + # dropped left a clean-nodes/dirty-edge response entirely unexamined, so this + # runs unconditionally; only the endpoint cascade depends on dropped ids. + def _endpoint_dropped(*values: "object") -> bool: + # Judge each endpoint on its own: a single unhashable `source` must not + # mask a `target` that really was dropped and leave the edge dangling. + for v in values: + try: + if v in dropped_ids: + return True + except TypeError: # unhashable endpoint from a malformed result + continue + return False + + def _members_dropped(members: "object") -> bool: + try: + return bool(dropped_ids & set(members or [])) + except TypeError: + return False + + edges_before = len(merged.get("edges", [])) + hyper_before = len(merged.get("hyperedges", [])) + merged["edges"] = [ + e for e in merged.get("edges", []) + if not _out_of_scope(e) and not _endpoint_dropped(e.get("source"), e.get("target")) + ] + merged["hyperedges"] = [ + h for h in merged.get("hyperedges", []) + if not _out_of_scope(h) and not _members_dropped(h.get("nodes")) + ] + merged["provenance_dropped_edges"] = edges_before - len(merged["edges"]) + merged["provenance_dropped_hyperedges"] = hyper_before - len(merged["hyperedges"]) + + if dropped_node_count: shown = ", ".join(sorted(Path(f).name for f in dropped_files)[:5]) more = f" (+{len(dropped_files) - 5} more)" if len(dropped_files) > 5 else "" + # Name the two failure modes separately: a misattribution points at a real + # file you can go read, whereas a fabricated path has nothing behind it and + # usually signals a prompt/backend problem rather than a corpus one. + breakdown = ", ".join( + f"{count} {reason}" + for reason, count in ( + ("mis-attributed to another corpus file", dropped_by_reason["misattributed"]), + ("attributed to a path that does not exist", dropped_by_reason["fabricated"]), + ) + if count + ) print( f"[graphify] WARNING: dropped {dropped_node_count} out-of-scope node(s) " f"attributed to file(s) not dispatched for extraction: {shown}{more}. " - "The model mis-attributed them to another corpus file; they were " - "excluded from the graph (#1895).", + f"({breakdown}.) They were excluded from the graph (#1895, #2217).", file=sys.stderr, ) covered: set[Path] = set() for n in merged.get("nodes", []): sf = n.get("source_file") - if sf: + # PRE-EXISTING on 0b2bd93, not introduced here: a non-string source_file + # (the #1631 malformed-result class) reached `Path()` and raised + # TypeError, aborting the run after every chunk had been paid for. It is + # guarded rather than left alone because the node filter above now guards + # the identical class two blocks up, and a crash between them would be + # incoherent. + if sf and isinstance(sf, (str, Path)): p = Path(sf) covered.add(p if p.is_absolute() else (root / p)) uncovered = sorted( diff --git a/tests/test_llm_backends.py b/tests/test_llm_backends.py index c1392ded5..babcb8724 100644 --- a/tests/test_llm_backends.py +++ b/tests/test_llm_backends.py @@ -962,6 +962,51 @@ def test_native_extraction_prompt_matches_skill_spec_on_hyperedges(): assert shared in llm._EXTRACTION_SYSTEM, "native prompt drifted from the skill hyperedge wording" +def test_native_extraction_prompt_states_the_source_file_rule(): + """#2217: source_file is provenance, so both extraction paths must forbid + inventing it. + + The skill path has carried a `source_file RULE` since #1366 ("copy the + FILE_LIST entry character-for-character"). The native/API prompt had no rule + at all — it showed `"source_file":"relative/path"` in the schema and left the + rest to the model, which is how a DeepSeek run came to reconstruct a + plausible repo path out of a node id quoted inside a document. + + Pin the contract rather than the wording: the rule must exist, must name the + path as the source of truth, and must forbid deriving it + from what the file merely mentions. + + Asserted against the RULE PARAGRAPH, not the whole prompt. Matching the whole + prompt is worthless here: the base prompt already contains "" + (the SECURITY paragraph), "node id" ("Node ID format: ...") and "citation" + ("EXTRACTED: relationship explicit in source (import, call, citation, ...)"), + so a rule saying the exact opposite — "pick whatever source_file seems most + plausible (basename is fine)" — passed the whole-prompt version green. + """ + spec = ( + Path(__file__).resolve().parents[1] + / "tools" / "skillgen" / "fragments" / "references" / "shared" / "extraction-spec.md" + ).read_text(encoding="utf-8") + assert "source_file RULE" in spec, "skill extraction-spec lost its source_file rule" + + for deep in (False, True): + prompt = llm._extraction_system(deep=deep) + assert "source_file RULE" in prompt, f"deep={deep}: native prompt has no source_file rule" + # Isolate the rule paragraph so nothing else in the prompt can satisfy it. + rule = prompt.split("source_file RULE", 1)[1].split("\n\n", 1)[0].lower() + + assert "untrusted_source" in rule, ( + f"deep={deep}: rule does not name the block the path must be copied from" + ) + assert "never" in rule, f"deep={deep}: rule states no prohibition" + # The failure mode #2217 reported: deriving provenance from something the + # document merely refers to, instead of from the dispatched path. + for forbidden in ("node id", "citation", "basename", "label"): + assert forbidden in rule, ( + f"deep={deep}: rule does not forbid inferring source_file from a {forbidden}" + ) + + # --- *_BASE_URL env overrides for kimi / gemini / deepseek (#1458) ------------- # BACKENDS reads the env at import time, so each case runs in a fresh interpreter # (subprocess) to avoid reload contamination of the test session. diff --git a/tests/test_provenance_invariant.py b/tests/test_provenance_invariant.py new file mode 100644 index 000000000..8efb4e406 --- /dev/null +++ b/tests/test_provenance_invariant.py @@ -0,0 +1,497 @@ +"""Tests for the semantic ``source_file`` provenance invariant (#2217). + +A semantic result's ``source_file`` is provenance: it is what tells a reader +which dispatched file a node came from. ``_out_of_scope`` (#1895) held only +attributions that resolved to an *existing* file outside the dispatched set, so +a path the model invented — one that exists nowhere — was admitted to the graph +while ``save_semantic_cache`` refused the identical attribution. These tests pin +the corrected rule: existence never decides admission, a *claimed* file does. +""" + +import warnings +from pathlib import Path +from unittest.mock import patch + +import pytest + +from graphify.cache import save_semantic_cache +from graphify.llm import _claims_a_file, extract_corpus_parallel + + +def _run(files, result, tmp_path): + """Drive extract_corpus_parallel with a faked per-chunk extraction result.""" + payload = {"input_tokens": 1, "output_tokens": 1, **result} + payload.setdefault("edges", []) + payload.setdefault("hyperedges", []) + with patch("graphify.llm.extract_files_direct", side_effect=lambda c, **k: dict(payload)): + return extract_corpus_parallel( + files, backend="kimi", root=tmp_path, + token_budget=None, chunk_size=len(files), max_concurrency=1, + ) + + +# ── _claims_a_file ──────────────────────────────────────────────────────────── + + +@pytest.mark.parametrize( + "value", + [ + "src/auth/session.py", # the #2217 shape: assembled repo path + "src\\auth\\session.py", # Windows separators + "docs/README.md", # known doc extension + "papers/paper.pdf", # known paper extension + "assets/diagram.png", # known image extension + "a/b/notes.DOCX", # case-insensitive suffix match + ], +) +def test_values_that_claim_a_file(value): + assert _claims_a_file(value) is True + + +@pytest.mark.parametrize( + "value", + [ + # Concept markers #1895 deliberately preserves. + "auth flow", + "authentication", + "React.Component", # dotted, but ".component" is not a corpus extension + "graphify v0.9.26", # version suffix must not read as an extension + "Section 3.2", + "", + " ", + # Slash-bearing prose. A separator alone must never imply a path: models + # routinely put these in source_file, and treating them as file claims + # deleted the nodes outright. + "N/A", + "TCP/IP", + "CI/CD", + "and/or", + "I/O", + "24/7", + "input/output", + "A/B testing", + "client/server architecture", + "read/write lock", + # A corpus suffix alone must not imply a path either — these are + # technology names, not files. + "Node.js", + "React.js", + "Vue.js", + "D3.js", + "Objective.C", + # Directory-shaped values. cache.py documents that subagent fragments + # really do carry a directory here; it is malformed provenance, not a + # fabricated file, and the base filter kept these. + "src/auth", + "src/auth/", + "docs/", + ], +) +def test_values_that_do_not_claim_a_file(value): + assert _claims_a_file(value) is False + + +def test_slash_bearing_prose_and_directories_survive_the_filter(tmp_path): + """End-to-end guard for the class above: these must reach the graph. + + A false positive here does not merely mislabel a node — it deletes it and + cascade-drops every edge that touches it. + """ + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + (tmp_path / "src").mkdir() + (tmp_path / "src" / "auth").mkdir() + + survivors = { + "na": "N/A", + "cicd": "CI/CD", + "tcpip": "TCP/IP", + "cs": "client/server architecture", + "nodejs": "Node.js", + "adir": "src/auth", + } + out = _run([doc], {"nodes": [ + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + *({"id": k, "source_file": v, "file_type": "concept"} for k, v in survivors.items()), + ]}, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"ok", *survivors} + assert out["out_of_scope_dropped"] == 0 + + +def test_filter_is_a_superset_of_the_1895_existence_check(tmp_path): + """The claim rule is an ADDITION to the existence check, never a replacement. + + A real corpus file that was not dispatched must still be dropped even when its + name carries no corpus extension (`Makefile`, `LICENSE`, `pyproject.toml`). + Replacing the existence arm silently re-admitted these while + `save_semantic_cache` went on refusing them — reopening, for that class, the + very graph-vs-cache divergence this change exists to close. + """ + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + for name in ("Makefile", "LICENSE", "pyproject.toml", "data.csv"): + (tmp_path / name).write_text("x\n", encoding="utf-8") + + out = _run([doc], {"nodes": [ + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + *({"id": n.lower(), "source_file": n, "file_type": "document"} + for n in ("Makefile", "LICENSE", "pyproject.toml", "data.csv")), + ]}, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"ok"} + # Real files, so they are misattributions (#1895), not fabrications. + assert out["provenance_dropped_misattributed"] == 4 + assert out["provenance_dropped_fabricated"] == 0 + + +def test_duplicate_id_keeps_the_surviving_copys_edges(tmp_path): + """Chunks are concatenated, so one id can appear twice in the merged result. + + If one copy is dropped and another survives, the id is still live. Pruning its + edges would leave a correctly-attributed node stripped of every relationship — + the hazard cache.py fixed for its own prune in #1916. + """ + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "dup", "source_file": "notes.md", "file_type": "document"}, + {"id": "dup", "source_file": "src/auth/session.py", "file_type": "code"}, + {"id": "peer", "source_file": "notes.md", "file_type": "document"}, + ], + "edges": [{"source": "dup", "target": "peer", "source_file": "notes.md"}], + "hyperedges": [{"id": "h", "nodes": ["dup", "peer"], "source_file": "notes.md"}], + }, tmp_path) + + assert out["provenance_dropped_fabricated"] == 1 + assert "dup" in {n["id"] for n in out["nodes"]} + assert len(out["edges"]) == 1, "surviving duplicate lost its edge" + assert len(out["hyperedges"]) == 1, "surviving duplicate lost its hyperedge" + + +def test_dirty_edge_is_filtered_even_when_no_node_is_dropped(tmp_path): + """The prompt holds edges to the same source_file rule as nodes, and the cache + groups them by their own source_file. Filtering them only when some node + happened to be dropped left a clean-nodes/dirty-edge response unexamined.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "a", "source_file": "notes.md", "file_type": "document"}, + {"id": "b", "source_file": "notes.md", "file_type": "document"}, + ], + "edges": [ + {"source": "a", "target": "b", "source_file": "notes.md"}, + {"source": "a", "target": "b", "source_file": "src/auth/session.py"}, + ], + "hyperedges": [ + {"id": "h_bad", "nodes": ["a", "b"], "source_file": "src/auth/session.py"}, + ], + }, tmp_path) + + assert out["out_of_scope_dropped"] == 0, "no node should have been dropped" + assert len(out["edges"]) == 1 + assert out["edges"][0]["source_file"] == "notes.md" + assert out["hyperedges"] == [] + assert out["provenance_dropped_edges"] == 1 + assert out["provenance_dropped_hyperedges"] == 1 + + +def test_unhashable_ids_do_not_abort_the_run(tmp_path): + """Untrusted results really do put lists where scalars belong (#1631). The + filter must degrade, not raise, after every chunk has already been paid for.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": ["a", "b"], "source_file": "src/auth/session.py", "file_type": "code"}, + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + ], + "edges": [{"source": ["a"], "target": "ok", "source_file": "notes.md"}], + }, tmp_path) + + assert "ok" in {str(n.get("id")) for n in out["nodes"]} + + +@pytest.mark.parametrize( + "spelling", + [ + " pkg/B.md ", # surrounding whitespace + "pkg/ B.md", # whitespace after the separator + "pkg\\B.md", # Windows separators on a POSIX host + " pkg\\B.md ", # both + ], +) +def test_alternate_spellings_of_a_dispatched_path_are_tolerated(tmp_path, spelling): + """Both halves of the decision must canonicalise identically. + + `_claims_a_file` flips backslashes and strips segments. If the dispatched-set + lookup does not, a node attributed to a genuinely dispatched file misses the + set, misses is_file()/is_dir(), and is deleted as "fabricated" by the one half + that did normalise — taking its edges with it. + + The path must be NESTED: a top-level ` notes.md ` can never reach the + fabricated arm (no separator means `_claims_a_file` is False), so it would + pass with the whole normalisation deleted. That earlier version of this test + was green against code that had been removed entirely. + """ + sub = tmp_path / "pkg" + sub.mkdir() + doc = sub / "B.md" + doc.write_text("b\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "ws", "source_file": spelling, "file_type": "document"}, + {"id": "peer", "source_file": "pkg/B.md", "file_type": "document"}, + ], + "edges": [{"source": "ws", "target": "peer", "source_file": "pkg/B.md"}], + }, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"ws", "peer"}, ( + f"{spelling!r} was not recognised as the dispatched pkg/B.md" + ) + assert out["provenance_dropped_fabricated"] == 0 + assert len(out["edges"]) == 1, "the node's edge was cascade-dropped with it" + + +def test_directory_attribution_that_looks_like_a_file_is_kept(tmp_path): + """Pins the `is_dir()` arm, which is otherwise unreachable in the suite. + + A value only reaches that arm when it BOTH claims a file and resolves to a + directory, so it needs a directory named like one. Deleting the arm must fail + a test rather than silently start dropping directory attributions. + """ + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + weird = tmp_path / "docs" / "guide.md" + weird.mkdir(parents=True) # a DIRECTORY named guide.md + + out = _run([doc], {"nodes": [ + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "dir", "source_file": "docs/guide.md", "file_type": "concept"}, + ]}, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"ok", "dir"} + assert out["out_of_scope_dropped"] == 0 + + +def test_url_shaped_source_file_is_not_a_fabrication(tmp_path): + """A URL is a `source_url`, not a corpus path. Base kept these; a scheme is + never evidence that a local file was claimed.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], {"nodes": [ + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "url", "source_file": "https://example.com/pkg/mod.py", "file_type": "document"}, + ]}, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"ok", "url"} + assert out["out_of_scope_dropped"] == 0 + + +def test_non_string_source_file_does_not_abort_the_run(tmp_path): + """The #1631 class again, on the source_file field rather than the id.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], {"nodes": [ + {"id": "ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "weird", "source_file": ["a/b.py"], "file_type": "document"}, + {"id": "num", "source_file": 42, "file_type": "document"}, + ]}, tmp_path) + + assert "ok" in {n["id"] for n in out["nodes"]} + + +# ── The #2217 defect ────────────────────────────────────────────────────────── + + +def test_invented_nonexistent_source_file_is_dropped(tmp_path, capsys): + """#2217: the model reconstructs a plausible repo path from a node id inside + a document. The path exists nowhere and was never dispatched, so the old + ``p.is_file()`` condition admitted it as a 'model-invented anchor'.""" + doc = tmp_path / "notes.md" + doc.write_text("see src_auth_session_validate for details\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "notes_ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "ghost", "source_file": "src/auth/session.py", "file_type": "code"}, + ], + }, tmp_path) + + ids = {n["id"] for n in out["nodes"]} + assert "ghost" not in ids, "invented source_file reached the merged graph (#2217)" + assert ids == {"notes_ok"} + assert out["out_of_scope_dropped"] == 1 + assert out["provenance_dropped_fabricated"] == 1 + assert out["provenance_dropped_misattributed"] == 0 + assert "does not exist" in capsys.readouterr().err + + +def test_invented_attribution_cascades_to_edges_and_hyperedges(tmp_path): + """A dropped node must not leave a dangling edge or hyperedge behind, and an + edge/hyperedge carrying the invented attribution itself is dropped too.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "notes_ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "also_ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "ghost", "source_file": "src/auth/session.py", "file_type": "code"}, + ], + "edges": [ + {"source": "notes_ok", "target": "also_ok", "source_file": "notes.md"}, + # endpoint was dropped + {"source": "notes_ok", "target": "ghost", "source_file": "notes.md"}, + # the edge itself carries the invented attribution + {"source": "notes_ok", "target": "also_ok", + "source_file": "src/auth/session.py"}, + ], + "hyperedges": [ + {"id": "h_ok", "nodes": ["notes_ok", "also_ok"], "source_file": "notes.md"}, + {"id": "h_ghost", "nodes": ["notes_ok", "ghost"], "source_file": "notes.md"}, + ], + }, tmp_path) + + node_ids = {n["id"] for n in out["nodes"]} + assert "ghost" not in node_ids + assert len(out["edges"]) == 1, f"dangling/invented edges survived: {out['edges']}" + assert out["edges"][0]["target"] == "also_ok" + assert [h["id"] for h in out["hyperedges"]] == ["h_ok"] + + +def test_misattributed_and_fabricated_are_counted_separately(tmp_path): + """The two failure modes need different fixes, so they get different counters: + a real file you can go read vs. a path with nothing behind it.""" + a = tmp_path / "A.md"; a.write_text("a\n", encoding="utf-8") + real_undispatched = tmp_path / "B.py" + real_undispatched.write_text("def b(): pass\n", encoding="utf-8") + + out = _run([a], { + "nodes": [ + {"id": "a_ok", "source_file": "A.md", "file_type": "document"}, + {"id": "mis", "source_file": "B.py", "file_type": "code"}, + {"id": "fab", "source_file": "src/nope.py", "file_type": "code"}, + ], + }, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"a_ok"} + assert out["provenance_dropped_misattributed"] == 1 + assert out["provenance_dropped_fabricated"] == 1 + assert out["out_of_scope_dropped"] == 2 + + +# ── What the invariant must NOT do ──────────────────────────────────────────── + + +def test_concept_marker_source_file_still_survives(tmp_path): + """Regression guard on the design choice. The literal invariant suggested in + #2217 — every non-empty source_file must resolve into the dispatched set — + deletes a concept node whose source_file is a bare phrase, which #1895 + deliberately kept (and which the #1895 test asserts). Only values that CLAIM + a file are held to the dispatched set.""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + + out = _run([doc], { + "nodes": [ + {"id": "notes_ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "auth_flow", "source_file": "auth flow", "file_type": "concept"}, + {"id": "no_anchor", "file_type": "concept"}, + ], + }, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"notes_ok", "auth_flow", "no_anchor"} + assert out["out_of_scope_dropped"] == 0 + + +def test_dispatched_files_are_never_dropped(tmp_path, capsys): + """A clean run must be untouched: no drops, no warning, counters at zero.""" + a = tmp_path / "A.md"; a.write_text("a\n", encoding="utf-8") + sub = tmp_path / "pkg"; sub.mkdir() + b = sub / "B.md"; b.write_text("b\n", encoding="utf-8") + + out = _run([a, b], { + "nodes": [ + {"id": "a_ok", "source_file": "A.md", "file_type": "document"}, + # nested attribution, given relative to root exactly as spec'd + {"id": "b_ok", "source_file": "pkg/B.md", "file_type": "document"}, + ], + }, tmp_path) + + assert {n["id"] for n in out["nodes"]} == {"a_ok", "b_ok"} + assert out["out_of_scope_dropped"] == 0 + assert out["provenance_dropped_fabricated"] == 0 + assert "out-of-scope" not in capsys.readouterr().err + + +# ── The property the bug violated ───────────────────────────────────────────── + + +def _cache_accepts(node: dict, doc: Path, tmp_path: Path, probe: Path) -> bool: + """Whether ``save_semantic_cache`` (#1757) will persist *node*. + + Uses the documented return value — the number of entries written — rather + than probing the on-disk layout, so the signal cannot silently go vacuous. + """ + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + return save_semantic_cache( + [dict(node)], [], [], root=tmp_path, cache_root=probe, + allowed_source_files=[doc], + ) > 0 + + +def test_file_claiming_attributions_agree_with_the_cache(tmp_path): + """The property whose violation caused #2217's non-convergence. + + ``save_semantic_cache`` (#1757) skips a group when ``not p.is_file() or p not + in allowed``; the merged-graph filter used to apply only the second half. A + node the graph admitted but the cache refused became a ghost: absent from the + cache, it was re-extracted on every ``--update``, minting a *different* ghost + each run. + + For every source_file that CLAIMS a file, graph admission and cache admission + must now be the same decision. (Bare concept markers are deliberately outside + this property: #1895 keeps them in the graph, while the cache — which is keyed + by real file — has always skipped them. That divergence is pre-existing, is + not a ghost, and is asserted separately below.)""" + doc = tmp_path / "notes.md" + doc.write_text("notes\n", encoding="utf-8") + real_undispatched = tmp_path / "other.py" + real_undispatched.write_text("x = 1\n", encoding="utf-8") + + file_claims = [ + {"id": "notes_ok", "source_file": "notes.md", "file_type": "document"}, + {"id": "ghost", "source_file": "src/auth/session.py", "file_type": "code"}, + {"id": "mis", "source_file": "other.py", "file_type": "code"}, + ] + concept = {"id": "concept", "source_file": "auth flow", "file_type": "concept"} + + out = _run([doc], {"nodes": [dict(n) for n in file_claims + [concept]]}, tmp_path) + survivors = {n["id"] for n in out["nodes"]} + + for i, node in enumerate(file_claims): + in_graph = node["id"] in survivors + in_cache = _cache_accepts(node, doc, tmp_path, tmp_path / f"probe{i}") + assert in_graph == in_cache, ( + f"{node['id']!r} ({node['source_file']!r}): graph={in_graph} " + f"cache={in_cache} — the #2217 divergence" + ) + + # Sanity: the probe distinguishes accept from refuse (guards against a + # vacuously-passing loop where the cache refuses everything). + assert _cache_accepts(file_claims[0], doc, tmp_path, tmp_path / "probe_ok") is True + assert _cache_accepts(file_claims[1], doc, tmp_path, tmp_path / "probe_bad") is False + + # The documented exception, pinned so it stays deliberate. + assert concept["id"] in survivors + assert _cache_accepts(concept, doc, tmp_path, tmp_path / "probe_concept") is False diff --git a/tests/test_worked_graph_provenance.py b/tests/test_worked_graph_provenance.py new file mode 100644 index 000000000..09edc1a55 --- /dev/null +++ b/tests/test_worked_graph_provenance.py @@ -0,0 +1,151 @@ +"""Provenance audit of the committed reference graphs under ``worked/``. + +These are real graphs from real corpora (``worked/rsl-siege-manager`` alone +carries 1,268 AST nodes and 618 LLM-produced ``rationale`` nodes), so they are +the only production-shaped semantic output in the tree — and therefore where a +fabricated-provenance regression (#2217) would first be visible. + +Every ``source_file`` that claims a file must be one the same run demonstrably +knew about: the manifest where one was committed, otherwise the set of files the +deterministic AST layer attributed nodes to. A path the model invented appears in +neither. + +The corpora themselves are not vendored, so this asserts internal consistency +rather than on-disk existence — which is the right test anyway, since #2217 is +precisely about existence being the wrong question. + +Scope note: this does NOT guard `_claims_a_file` against over-tightening. That +function only decides which values get checked, and every attribution in the one +non-vacuous fixture is a real manifest path, so a stricter `_claims_a_file` could +never produce an offender here. Over-tightening is covered by +`test_values_that_do_not_claim_a_file` and by +`tests/test_chunking.py::test_out_of_scope_nodes_are_dropped_from_merged_result`. +The claim floor below is the only signal this file offers on that axis. +""" + +import json +from pathlib import Path + +import pytest + +from graphify.llm import _claims_a_file + +WORKED = Path(__file__).resolve().parents[1] / "worked" +GRAPHS = sorted(WORKED.glob("*/graph.json")) if WORKED.is_dir() else [] + +# Fixtures whose audited-claim count must not silently collapse. If a future +# change stops classifying ordinary attributions as file claims, this floor drops +# and the test fails rather than passing on an empty check. +CLAIM_FLOOR = {"rsl-siege-manager": 1800} + +pytestmark = pytest.mark.skipif(not GRAPHS, reason="worked/ reference graphs not present") + + +def _norm(value: str) -> str: + """Case- and separator-insensitive path key. + + Uses ``removeprefix("./")`` rather than ``lstrip("./")``: lstrip strips a + character SET, so it eats the leading dot of ``.github/workflows/ci.yml`` and + swallows ``../`` prefixes whole, making legitimate attributions look invented. + """ + return str(value).replace("\\", "/").lower().removeprefix("./") + + +def _strip_common_prefix(paths: "set[str]") -> "set[str]": + """Reduce absolute manifest keys to corpus-relative form. + + The manifest holds absolute paths from the machine that built the graph, while + nodes hold repo-relative ones. Stripping the longest shared directory prefix + lets the two be compared by exact equality — which matters, because an + ``endswith`` fallback would also accept a bare basename, and "inferred it from + a basename" is exactly the fabrication the extraction prompt now forbids. + """ + if len(paths) < 2: + return set(paths) + split = [p.split("/") for p in paths] + common = 0 + for parts in zip(*split): + if len(set(parts)) != 1: + break + common += 1 + if not common: + return set(paths) + return {"/".join(p[common:]) for p in split} + + +def _ground_truth(graph_dir: Path, nodes: list[dict]) -> "tuple[set[str], bool]": + """(known files, whether the truth is independent of the audited nodes). + + A committed ``manifest.json`` lists the files the run actually walked, so it + is independent evidence and every node can be judged against it. + + Without one, the only available reference is the AST layer's own + attributions. That is still ground truth *for the semantic layer* — the AST + engine is deterministic and never invents a path — but it cannot judge code + nodes, because a fabricated code node would contribute its own path to the + reference set and validate itself. Callers must restrict the audit + accordingly; the flag says which regime applies. + """ + manifest = graph_dir / "manifest.json" + if manifest.exists(): + keys = {_norm(k) for k in json.loads(manifest.read_text(encoding="utf-8"))} + return _strip_common_prefix(keys), True + return { + _norm(n["source_file"]) + for n in nodes + if n.get("file_type") == "code" and n.get("source_file") + }, False + + +def test_norm_preserves_dot_directories(): + """Regression guard for the lstrip character-set bug.""" + assert _norm(".github/workflows/ci.yml") == ".github/workflows/ci.yml" + assert _norm("./src/App.tsx") == "src/app.tsx" + assert _norm("..\\pkg\\mod.py") == "../pkg/mod.py" + + +@pytest.mark.parametrize("graph_path", GRAPHS, ids=lambda p: p.parent.name) +def test_no_node_claims_a_file_the_run_never_saw(graph_path: Path): + nodes = json.loads(graph_path.read_text(encoding="utf-8")).get("nodes", []) + assert nodes, f"{graph_path} has no nodes — fixture is empty" + known, independent = _ground_truth(graph_path.parent, nodes) + assert known, f"{graph_path}: could not establish the known-file set" + + # With only the AST layer as reference, audit the semantic layer alone — + # judging code nodes against a set they define would be circular. + audited = [n for n in nodes if independent or n.get("file_type") != "code"] + claims = [ + n for n in audited + if n.get("source_file") and _claims_a_file(n["source_file"]) + ] + # The floor is asserted BEFORE the skip, deliberately. Checking it afterwards + # made it unreachable in the one case it exists for: if `_claims_a_file` + # collapsed entirely, `claims` would be empty, the skip would fire first, and + # the only signal-bearing fixture would report as skipped rather than failed. + floor = CLAIM_FLOOR.get(graph_path.parent.name) + if floor is not None: + assert len(claims) >= floor, ( + f"{graph_path.parent.name}: only {len(claims)} file-claiming attribution(s) " + f"checked, expected >= {floor}. Either the fixture changed or " + "_claims_a_file stopped recognising ordinary corpus paths." + ) + + if not claims: + # Nothing to check. Skip rather than report green: a fixture whose audited + # nodes carry no file-claiming source_file (karpathy-repos) would + # otherwise pass vacuously, which is the failure mode this file exists to + # avoid. + pytest.skip( + f"{graph_path.parent.name}: {len(audited)} audited node(s), " + "none carrying a file-claiming source_file" + ) + + offenders = [ + (n.get("id"), n.get("file_type"), n["source_file"]) + for n in claims + if _norm(n["source_file"]) not in known + ] + assert not offenders, ( + f"{graph_path.parent.name}: {len(offenders)} of {len(claims)} checked node(s) " + f"claim a file the run never saw (fabricated provenance, #2217): {offenders[:5]}" + )