diff --git a/ast_rag/services/graph_updater_service.py b/ast_rag/services/graph_updater_service.py index cf954fc..d2dd3aa 100644 --- a/ast_rag/services/graph_updater_service.py +++ b/ast_rag/services/graph_updater_service.py @@ -345,17 +345,19 @@ def update_from_git( if lang is None: continue - # Load new content (may no longer exist if deleted) - if not os.path.exists(file_path): - # File was deleted: all old nodes from this file need expiration. - # For PoC: we mark them by querying Neo4j. + rel_path = os.path.relpath(file_path, repo_path) + + # Read the new side at new_commit, not from disk: this writes to + # the graph, so a dirty tree would persist code that never existed + # at the commit it gets stamped with. Uncommitted work has its own + # path in get_workspace_diff. + new_source = _read_blob(repo, new_commit, rel_path) + if new_source is None: + # Absent at new_commit: expire everything this file defined. _expire_file_nodes(driver, file_path, new_commit) continue - with open(file_path, "rb") as fh: - new_source = fh.read() - - # Parse the NEW version + # Parse the version at new_commit new_tree = pm.parse_file(file_path, source=new_source) if new_tree is None: continue @@ -365,7 +367,6 @@ def update_from_git( ) # Load old version from git at old_commit - rel_path = os.path.relpath(file_path, repo_path) old_source_bytes = _read_blob(repo, old_commit, rel_path) old_nodes: list[ASTNode] = [] old_edges: list[ASTEdge] = [] @@ -509,11 +510,13 @@ def compute_diff_for_commits( if lang is None: continue - # Load new content (may no longer exist if deleted) - if not os.path.exists(file_path): - # File was deleted - we still process it to mark nodes for expiration - # Load old version from git - rel_path = os.path.relpath(file_path, repo_path) + rel_path = os.path.relpath(file_path, repo_path) + + # Both sides come out of the object store. Reading the new side from + # disk would describe the working tree while stamping it to_commit. + new_source = _read_blob(repo, to_commit, rel_path) + if new_source is None: + # Absent at to_commit: expire whatever it defined at from_commit. old_source_bytes = _read_blob(repo, from_commit, rel_path) if old_source_bytes is not None: old_tree = pm.parse_file(file_path, source=old_source_bytes) @@ -532,10 +535,7 @@ def compute_diff_for_commits( agg_diff.deleted_edge_ids.extend(file_diff.deleted_edge_ids) continue - with open(file_path, "rb") as fh: - new_source = fh.read() - - # Parse the NEW version + # Parse the version at to_commit new_tree = pm.parse_file(file_path, source=new_source) if new_tree is None: continue @@ -543,7 +543,6 @@ def compute_diff_for_commits( new_edges = pm.extract_edges(new_tree, new_nodes, file_path, lang, new_source, to_commit) # Load old version from git at from_commit - rel_path = os.path.relpath(file_path, repo_path) old_source_bytes = _read_blob(repo, from_commit, rel_path) old_nodes: list[ASTNode] = [] old_edges: list[ASTEdge] = [] diff --git a/tests/test_diff_reads_the_named_commit.py b/tests/test_diff_reads_the_named_commit.py new file mode 100644 index 0000000..cefa997 --- /dev/null +++ b/tests/test_diff_reads_the_named_commit.py @@ -0,0 +1,116 @@ +"""``A..B`` must describe B, not whatever is checked out right now. + +Both git-diff paths read the *old* side from the object store with +``_read_blob`` and the *new* side with ``open(file_path)`` -- the working +tree. ``compute_diff_for_commits`` promises "added_nodes: New AST nodes in +to_commit", and ``ast-rag update`` takes ``--from-commit`` and ``--to-commit`` +as required options, so a user naming any pair other than +``..HEAD`` on a clean tree gets the current source parsed and +stamped with ``to_commit``. + +``update_from_git`` has the same line, and that one writes to Neo4j: the +graph ends up holding code that never existed at the commit it is labelled +with. Uncommitted work already has its own path in ``get_workspace_diff``, so +the two are not meant to be the same thing. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from ast_rag.services.graph_updater_service import compute_diff_for_commits + +pytest.importorskip("git", reason="GitPython is needed to build the fixture repo") + + +def _git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", *args], + cwd=repo, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def _commit(repo: Path, message: str) -> str: + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", message) + return _git(repo, "rev-parse", "HEAD") + + +@pytest.fixture +def repo(tmp_path: Path) -> dict: + """A repo where `alpha` sits on a different line in A, B and the tree.""" + root = tmp_path / "repo" + root.mkdir() + _git(root, "init", "-q") + _git(root, "config", "user.email", "test@example.com") + _git(root, "config", "user.name", "Test") + + module = root / "mod.py" + + # A: alpha on line 1 + module.write_text("def alpha():\n return 1\n") + commit_a = _commit(root, "a") + + # B: one comment pushes alpha to line 2, and the body changes so the + # node is genuinely part of the diff rather than untouched. + module.write_text("# header\ndef alpha():\n return 2\n") + commit_b = _commit(root, "b") + + # Working tree only: five more comments push alpha to line 7. Never committed. + module.write_text("# header\n# 1\n# 2\n# 3\n# 4\n# 5\ndef alpha():\n return 2\n") + + return {"path": str(root), "a": commit_a, "b": commit_b, "file": module} + + +def _alpha(diff) -> object: + nodes = [ + n + for n in (*diff.added_nodes, *diff.updated_nodes) + if n.name == "alpha" and n.kind.value in {"Function", "Method"} + ] + assert nodes, ( + "diff reported no `alpha` node at all; " + f"added={[n.name for n in diff.added_nodes]} " + f"updated={[n.name for n in diff.updated_nodes]}" + ) + return nodes[0] + + +def test_working_tree_is_not_mistaken_for_the_target_commit(repo): + """A..B must parse B's blob, not the file on disk.""" + diff = compute_diff_for_commits(repo["path"], repo["a"], repo["b"]) + alpha = _alpha(diff) + + assert alpha.start_line == 2, ( + f"`alpha` is on line 2 at to_commit and line 7 in the working tree; " + f"the diff reported line {alpha.start_line}, so it parsed the working tree" + ) + + +def test_diff_is_stable_while_the_working_tree_moves(repo): + """The same A..B must not change answer when an unrelated edit lands.""" + before = _alpha(compute_diff_for_commits(repo["path"], repo["a"], repo["b"])).start_line + + repo["file"].write_text("# a\n# b\n# c\n" + repo["file"].read_text()) + after = _alpha(compute_diff_for_commits(repo["path"], repo["a"], repo["b"])).start_line + + assert before == after, ( + f"editing an uncommitted file changed the reported diff of two fixed " + f"commits ({before} -> {after})" + ) + + +def test_source_text_comes_from_the_target_commit(repo): + """The body parsed must be B's body, not the tree's.""" + diff = compute_diff_for_commits(repo["path"], repo["a"], repo["b"]) + alpha = _alpha(diff) + + assert alpha.end_line == 3, ( + f"`alpha` spans lines 2-3 at to_commit; got {alpha.start_line}-{alpha.end_line}" + )