From 2936f85bb098fafaf2700985f50008ac3c506c53 Mon Sep 17 00:00:00 2001 From: Rohit Behera <126186063+r0h1tb@users.noreply.github.com> Date: Tue, 25 Aug 2026 02:10:09 +0530 Subject: [PATCH] fix(diff): read the target commit's blob, not the working tree Both git-diff paths read the old side from the object store and the new side with `open(file_path)`. `compute_diff_for_commits` promises "added_nodes: New AST nodes in to_commit", and `ast-rag update` takes --from-commit/--to-commit as required options, so any pair other than ..HEAD on a clean tree parses the current source and stamps it with to_commit. `update_from_git` has the same line and writes to Neo4j, so 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`. Read the new side with `_read_blob` at the target commit. Absence there, not absence on disk, now decides the deletion branch. --- ast_rag/services/graph_updater_service.py | 37 ++++--- tests/test_diff_reads_the_named_commit.py | 116 ++++++++++++++++++++++ 2 files changed, 134 insertions(+), 19 deletions(-) create mode 100644 tests/test_diff_reads_the_named_commit.py 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}" + )