vcs: follow a rename to the end, and never name a path that is not there

src/docproof/vcs.py

@@ -29,6 +29,12 @@
 TIMEOUT = 30
 
 
+# How far `moved_to` will follow a chain of renames before giving up. Ten is far past
+# anything real - the worst case in the corpus is two - and it exists only so a rename cycle
+# costs a bounded number of `git show` calls rather than a hang.
+_MAX_RENAME_HOPS = 10
+
+
 def _run(args: list[str], cwd: Path, stdin: str | None = None) -> tuple[int, str, str]:
     try:
         finished = subprocess.run(
@@ -163,6 +169,20 @@ def deleted(self, path: str) -> tuple[str, str, str] | None:
             return None
         return parts[0], parts[1], parts[2]
 
+    def _renamed_in(self, path: str, commit: str) -> str | None:
+        """One hop: where `commit` put `path`, if git called that deletion a rename."""
+        if not self.available or self.shallow:
+            return None
+        code, out, _ = _run(["git", "show", "-M", "--name-status", "--format=", commit], self.root)
+        if code != 0:
+            return None
+        for line in out.split("\n"):
+            parts = line.split("\t")
+            # `R100\told\tnew`. The similarity score rides on the status, so `startswith`.
+            if len(parts) == 3 and parts[0].startswith("R") and parts[1] == path:
+                return parts[2]
+        return None
+
     def moved_to(self, path: str, commit: str) -> str | None:
         """Where the deleting commit PUT it, when git calls the deletion a rename.
 
@@ -179,17 +199,42 @@ def moved_to(self, path: str, commit: str) -> str | None:
 
         This changes no verdict. It is the difference between a finding a reader has to
         investigate and one they can act on.
+
+        **FOLLOWED TO THE END, and it was one hop until 2026-08-18.** A file can be renamed
+        more than once, and reporting the first hop names a path that may itself be gone -
+        which is the worst thing this function can do, because the whole point of it is to
+        save the reader a search and it would be sending them somewhere empty.
+
+        Found on `open-gsd/gsd-core`. `docs/CONFIGURATION.md` cites
+        `sdk/shared/model-catalog.json` and the chain is two hops:
+
+            11918dcc  sdk/shared/model-catalog.json      -> get-shit-done/bin/shared/model-catalog.json
+            463cffd8  get-shit-done/bin/shared/...json   -> gsd-core/bin/shared/model-catalog.json
+
+        The old answer was the middle one. `get-shit-done/` has **zero** files at HEAD, so
+        the advice pointed into a directory that no longer exists anywhere in the tree.
+
+        So the invariant is now the one a reader assumes: **a destination is only named if it
+        is there now.** If the chain dead-ends somewhere that is not tracked, this returns
+        None and the finding says "deleted ... and never restored", which is true, instead of
+        naming a phantom. Bounded, because a rename cycle is cheaper to survive than to prove
+        impossible.
         """
-        if not self.available or self.shallow:
-            return None
-        code, out, _ = _run(["git", "show", "-M", "--name-status", "--format=", commit], self.root)
-        if code != 0:
+        destination = self._renamed_in(path, commit)
+        if destination is None:
             return None
-        for line in out.split("\n"):
-            parts = line.split("\t")
-            # `R100\told\tnew`. The similarity score rides on the status, so `startswith`.
-            if len(parts) == 3 and parts[0].startswith("R") and parts[1] == path:
-                return parts[2]
+        seen = {path, destination}
+        for _ in range(_MAX_RENAME_HOPS):
+            if self.tracks(destination):
+                return destination
+            onward = self.deleted(destination)
+            if onward is None:
+                return None
+            following = self._renamed_in(destination, onward[0])
+            if following is None or following in seen:
+                return None
+            seen.add(following)
+            destination = following
         return None
 
     def claim_introduced_after(self, doc: str, subject: str, commit: str) -> bool | None:

tests/test_rename_chain.py

@@ -0,0 +1,122 @@
+"""`moved to X` has to mean X is there now.
+
+`Git.moved_to` looked at the deleting commit and stopped. A file can be renamed more than
+once, and reporting the first hop names a path that may itself be gone - which is the worst
+thing that function can do, because its entire purpose is to save the reader a search and it
+would be sending them somewhere empty.
+
+Found on `open-gsd/gsd-core`, where `docs/CONFIGURATION.md` cites
+`sdk/shared/model-catalog.json` and the chain is two hops:
+
+    11918dcc  sdk/shared/model-catalog.json     -> get-shit-done/bin/shared/model-catalog.json
+    463cffd8  get-shit-done/bin/shared/...json  -> gsd-core/bin/shared/model-catalog.json
+
+The reported answer was the middle one, and `get-shit-done/` has zero files at that
+repository's HEAD.
+
+These build their own history rather than using `make_repo`, because the shape under test is
+two renames in two separate commits and pairing only happens within one commit - a fixture
+that cannot produce that cannot test this.
+"""
+
+from __future__ import annotations
+
+import subprocess
+from pathlib import Path
+
+import pytest
+
+from docproof.vcs import Git
+
+
+def _git(repo: Path, *args: str) -> None:
+    subprocess.run(
+        ["git", *args],
+        cwd=repo,
+        capture_output=True,
+        check=True,
+        timeout=60,
+        env={
+            "GIT_AUTHOR_NAME": "docproof tests",
+            "GIT_AUTHOR_EMAIL": "test@example.invalid",
+            "GIT_COMMITTER_NAME": "docproof tests",
+            "GIT_COMMITTER_EMAIL": "test@example.invalid",
+            "GIT_AUTHOR_DATE": "2026-01-01T00:00:00",
+            "GIT_COMMITTER_DATE": "2026-01-01T00:00:00",
+            "PATH": __import__("os").environ.get("PATH", ""),
+            "SYSTEMROOT": __import__("os").environ.get("SYSTEMROOT", ""),
+        },
+    )
+
+
+@pytest.fixture
+def repo(tmp_path: Path) -> Path:
+    root = tmp_path / "chain"
+    root.mkdir()
+    _git(root, "init", "-q")
+    # Content big enough that git scores the rename at R100 rather than guessing.
+    body = "\n".join(f"line {n} of a file worth following" for n in range(40)) + "\n"
+    first = root / "sdk" / "shared"
+    first.mkdir(parents=True)
+    (first / "catalog.json").write_text(body, encoding="utf-8")
+    _git(root, "add", "-A")
+    _git(root, "commit", "-qm", "one", "--no-gpg-sign")
+    return root
+
+
+def _move(repo: Path, old: str, new: str, message: str) -> None:
+    (repo / new).parent.mkdir(parents=True, exist_ok=True)
+    _git(repo, "mv", old, new)
+    _git(repo, "commit", "-qm", message, "--no-gpg-sign")
+
+
+def test_two_hops_reports_where_the_file_actually_is(repo: Path) -> None:
+    """The real shape, and the whole reason this exists."""
+    _move(repo, "sdk/shared/catalog.json", "middle/bin/shared/catalog.json", "retire the sdk seam")
+    _move(repo, "middle/bin/shared/catalog.json", "final/bin/shared/catalog.json", "rename middle")
+
+    git = Git(root=repo)
+    receipt = git.deleted("sdk/shared/catalog.json")
+    assert receipt is not None
+    destination = git.moved_to("sdk/shared/catalog.json", receipt[0])
+
+    assert destination == "final/bin/shared/catalog.json"
+    # The invariant, stated as an assertion rather than trusted: whatever is named exists.
+    assert git.tracks(destination)
+
+
+def test_one_hop_still_works(repo: Path) -> None:
+    """The behaviour that was already right must survive the fix."""
+    _move(repo, "sdk/shared/catalog.json", "kept/catalog.json", "move once")
+
+    git = Git(root=repo)
+    receipt = git.deleted("sdk/shared/catalog.json")
+    assert receipt is not None
+    assert git.moved_to("sdk/shared/catalog.json", receipt[0]) == "kept/catalog.json"
+
+
+def test_a_chain_that_ends_in_a_real_deletion_names_nothing(repo: Path) -> None:
+    """Renamed, then genuinely deleted.
+
+    The honest answer is None, so the finding reads "deleted ... and never restored" - which
+    is true - instead of pointing at an intermediate path that is no longer there. Naming a
+    phantom is worse than saying less.
+    """
+    _move(repo, "sdk/shared/catalog.json", "middle/catalog.json", "move it")
+    _git(repo, "rm", "-q", "middle/catalog.json")
+    _git(repo, "commit", "-qm", "and now really delete it", "--no-gpg-sign")
+
+    git = Git(root=repo)
+    receipt = git.deleted("sdk/shared/catalog.json")
+    assert receipt is not None
+    assert git.moved_to("sdk/shared/catalog.json", receipt[0]) is None
+
+
+def test_a_file_that_was_never_renamed_reports_nothing(repo: Path) -> None:
+    _git(repo, "rm", "-q", "sdk/shared/catalog.json")
+    _git(repo, "commit", "-qm", "plain deletion", "--no-gpg-sign")
+
+    git = Git(root=repo)
+    receipt = git.deleted("sdk/shared/catalog.json")
+    assert receipt is not None
+    assert git.moved_to("sdk/shared/catalog.json", receipt[0]) is None