diff --git a/src/docproof/vcs.py b/src/docproof/vcs.py index 61a3fde..3249377 100644 --- a/src/docproof/vcs.py +++ b/src/docproof/vcs.py @@ -383,6 +383,93 @@ def _copied_from(self, destination: str, source: str) -> str | None: return commit return None + def emptied_and_stayed(self, directory: str) -> str | None: + """The commit that took the last file out of a directory, if it never refilled. + + **A guard whose stated intent and behaviour disagreed.** `paths.py` skips a claim + written with a trailing slash when the directory's parent is tracked, because git + cannot represent an empty directory - `PostHog/posthog-python`'s RELEASING.md says + changesets live in `.sampo/changesets/`, that directory is emptied by every release + as the bot consumes them, and reporting it argues with a lifecycle. Right. + + Its comment then claims the tracked-parent requirement "keeps the case this must + still catch: a documented directory whose entire tree really did go". It keeps none + of them, because a wholly removed subdirectory of a live tree has a tracked parent + too. `stacklok/toolhive` documents `pkg/container/verifier/` for Sigstore + verification; `pkg/container` is alive and `verifier` went in `7095e8e1` + *"Remove /verifier in favour of one coming from toolhive-core"*. + + **Measured across the 47 clones of sweep batches 10 and 11**, every `--show-skips` + run captured to disk: the guard silences **38 directory claims, of which 31 were + never tracked at all** - promises, or places the reader creates, and the guard is + right about every one - **and 7 were populated and wholly emptied**, in four + repositories. None of the 38 is the churning case in this corpus, which is the shape + the guard was written for and which `composio/.changeset` confirms is real. + + So the skip keeps its reason and gains a receipt, which is the soundness rule the + rest of this verifier already runs on: a path is only called broken when the + repository can be shown to have HAD it and dropped it. + + THE REPLAY RUNS FORWARDS, and the first version of the measurement did not. Walking + newest-first and calling it a refill on the first add after a delete marks every + directory that was created before it was deleted, which is all of them; that run + reported zero emptied directories and put toolhive's verifier in the churn column. + Nothing about the output looked wrong. So: replay oldest-first, count how many times + the live set falls to empty, and only once means it stayed empty. + """ + code, out, _ = _run( + [ + "git", + "log", + "--reverse", + "-M", + "--name-status", + "--format=commit %h", + "--", + directory, + ], + self.root, + ) + if code != 0 or not out.strip(): + return None + + wanted = directory.strip("/") + prefix = wanted + "/" + + def inside(path: str) -> bool: + return path == wanted or path.startswith(prefix) + + live: set[str] = set() + commit: str | None = None + emptyings: list[str] = [] + refilled = False + for line in out.splitlines(): + if line.startswith("commit "): + commit = line[len("commit ") :].strip() + continue + parts = line.split("\t") + if len(parts) < 2 or commit is None: + continue + was_empty = not live + status = parts[0][:1] + if status == "D": + live.discard(parts[1]) + elif status in {"R", "C"} and len(parts) == 3: + if inside(parts[1]): + live.discard(parts[1]) + if inside(parts[2]): + live.add(parts[2]) + elif inside(parts[1]): + live.add(parts[1]) + if live and was_empty and emptyings: + refilled = True + elif not live and not was_empty: + emptyings.append(commit) + + if refilled or not emptyings: + return None + return emptyings[-1] + def claim_introduced_after(self, doc: str, subject: str, commit: str) -> bool | None: """Whether this document first mentioned `subject` *after* `commit` removed it. diff --git a/src/docproof/verifiers/paths.py b/src/docproof/verifiers/paths.py index c1aa0de..0f0f95a 100644 --- a/src/docproof/verifiers/paths.py +++ b/src/docproof/verifiers/paths.py @@ -550,7 +550,18 @@ def _judge(self, project: Project, claim: Claim, git: Git) -> Finding: f"instruction rather than a claim that this repository has it", ) - if claim.subject.rstrip().endswith("/") and git.tracks(str(PurePosixPath(subject).parent)): + # **UNLESS git can show it was populated and wholly emptied.** The comment above says + # the tracked-parent requirement keeps the case of a directory whose entire tree + # really did go; it keeps none of them, because a removed subdirectory of a live tree + # has a tracked parent too. Measured over 47 clones: this guard silences 38 directory + # claims, 31 never tracked at all and 7 populated and emptied for good. See + # `Git.emptied_and_stayed` - the skip keeps its reason and gains the same receipt + # every other verdict in this file runs on. + if ( + claim.subject.rstrip().endswith("/") + and git.tracks(str(PurePosixPath(subject).parent)) + and not git.emptied_and_stayed(subject) + ): return self.skip( claim, f"`{subject}` is written as a directory and its parent is tracked. Git " diff --git a/tests/test_emptied_directory.py b/tests/test_emptied_directory.py new file mode 100644 index 0000000..b53f331 --- /dev/null +++ b/tests/test_emptied_directory.py @@ -0,0 +1,149 @@ +"""An empty directory and a removed one look the same to git, until you replay the history. + +`paths.py` skips a claim written with a trailing slash when the directory's parent is +tracked. The reason is real: git stores files, not directories, so a directory with no files +is indistinguishable from one that never existed - and `PostHog/posthog-python`'s +RELEASING.md says changesets live in `.sampo/changesets/`, a directory every release empties +as the bot consumes them. The sentence says where `sampo add` PUTS files. Reporting it argues +with a lifecycle. + +The guard's own comment then claimed the tracked-parent requirement "keeps the case this must +still catch: a documented directory whose entire tree really did go". It keeps none of them. +A removed SUBdirectory of a live tree has a tracked parent too: + + stacklok/toolhive docs/arch/06-registry-system.md:864 `pkg/container/verifier/` + 7095e8e1 "Remove /verifier in favour of one coming from toolhive-core" + +`pkg/container` is alive. `verifier` is gone, and the tool said nothing. + +MEASURED over the 47 clones of sweep batches 10 and 11, every `--show-skips` run captured to +disk rather than sampled: the guard silences **38 directory claims, 31 of them never tracked +at all** - promises, and places the reader creates, and the guard is right about all 31 - +**and 7 populated and wholly emptied**, across toolhive, hive, onyx and openmed. + +The test for the posthog case builds its own history because the shape under test is a +directory that goes empty and REFILLS, and a fixture that commits once and deletes once +cannot express it. That distinction is the whole rule, and the previous test asserted the +posthog verdict over a history posthog does not have. +""" + +from __future__ import annotations + +import os +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": os.environ.get("PATH", ""), + "SYSTEMROOT": os.environ.get("SYSTEMROOT", ""), + }, + ) + + +def _add(repo: Path, path: str, message: str) -> None: + target = repo / path + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text("contents\n", encoding="utf-8") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", message, "--no-gpg-sign") + + +def _remove(repo: Path, path: str, message: str) -> None: + _git(repo, "rm", "-q", path) + _git(repo, "commit", "-qm", message, "--no-gpg-sign") + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + root = tmp_path / "tree" + root.mkdir() + _git(root, "init", "-q") + _add(root, "keep/anchor.txt", "something so the repository is not empty") + return root + + +def test_the_posthog_case_a_directory_that_refills(repo: Path) -> None: + """Filled, consumed, filled, consumed. This is what a changeset directory does. + + Verified against the real repository rather than only the fixture: at the clone on disk + `PostHog/posthog-python` has a file under `.sampo/changesets/` again, and + `emptied_and_stayed` returns None for it. + """ + _add(repo, ".sampo/changesets/gallant-prince.md", "add a changeset") + _remove(repo, ".sampo/changesets/gallant-prince.md", "release v7.39.0, consume changesets") + _add(repo, ".sampo/changesets/brave-otter.md", "add another changeset") + _remove(repo, ".sampo/changesets/brave-otter.md", "release v7.39.1, consume changesets") + + assert Git(root=repo).emptied_and_stayed(".sampo/changesets") is None + + +def test_the_toolhive_case_a_subsystem_removed_under_a_live_parent(repo: Path) -> None: + """The case the guard's comment claimed to keep and did not. + + Three files added over three commits, then all of them removed at once, and the parent + `pkg/container` still holds other code. That is a subsystem leaving, and every one of + the seven found in the corpus has this shape. + """ + _add(repo, "pkg/container/runtime.go", "the container package") + _add(repo, "pkg/container/verifier/sigstore.go", "add sigstore verification") + _add(repo, "pkg/container/verifier/cosign.go", "add cosign verification") + _git(repo, "rm", "-q", "-r", "pkg/container/verifier") + _git( + repo, + "commit", + "-qm", + "Remove /verifier in favour of one coming from toolhive-core", + "--no-gpg-sign", + ) + + git = Git(root=repo) + # The premise: the parent is alive, which is exactly why the old guard fired. + assert git.tracks("pkg/container") + assert git.emptied_and_stayed("pkg/container/verifier") is not None + + +def test_a_directory_that_never_existed_names_nothing(repo: Path) -> None: + """31 of the 38 silenced claims are this, and the guard is right about every one of them. + + A documented directory git has never tracked a file under is a promise, or a place the + reader is told to create. There is no receipt and there must be no finding. + """ + assert Git(root=repo).emptied_and_stayed("docs/changesets") is None + + +def test_a_directory_that_still_has_files_names_nothing(repo: Path) -> None: + _add(repo, "pkg/live/thing.go", "a package that is still here") + assert Git(root=repo).emptied_and_stayed("pkg/live") is None + + +def test_emptied_then_refilled_then_emptied_is_still_churn(repo: Path) -> None: + """The boundary, because a single trailing emptying is what the rule keys on. + + A directory that has been empty before and filled again is a lifecycle whatever state it + happens to be in at HEAD, and reporting it on the down-swing would be reporting the same + directory differently depending on when the run happened. + """ + _add(repo, "fragments/one.md", "first fragment") + _remove(repo, "fragments/one.md", "consume it") + _add(repo, "fragments/two.md", "second fragment") + _remove(repo, "fragments/two.md", "consume that one too") + + assert Git(root=repo).emptied_and_stayed("fragments") is None diff --git a/tests/test_paths.py b/tests/test_paths.py index ca67ff3..61ca0eb 100644 --- a/tests/test_paths.py +++ b/tests/test_paths.py @@ -544,17 +544,22 @@ def test_a_file_that_moved_under_src_did_not_vanish(make_repo: Callable[..., Pat assert "moved it under" in detail -def test_the_posthog_case_an_empty_directory_is_not_a_deleted_one(make_repo: Callable[..., Path]): +def test_a_documented_directory_git_never_tracked_is_not_drift(make_repo: Callable[..., Path]): """Git stores files, not directories, so it cannot tell empty from absent. - `PostHog/posthog-python`'s RELEASING.md says changesets must live in - `.sampo/changesets/`. The last file under it was deleted in `0fc7ec6` — the release bot - consuming a changeset on the v7.39.1 release, which is the directory's normal lifecycle, - not its removal. The sentence says where `sampo add` PUTS files and is still true. + **This used to be the posthog case and the fixture could not express it.** It built one + file and deleted it, which is not what `.sampo/changesets/` does — that directory is + filled and consumed on every release, and a fixture that commits once and removes once + has no lifecycle in it at all. The real shape now lives in + `tests/test_emptied_directory.py`, which builds the refill, and it was checked against + the live repository as well. + + What stays here is the case that needs no history: a directory the project documents and + git has never tracked a file under. **31 of the 38 claims this guard silences across the + 47-clone corpus are exactly this**, and there is no receipt to report. """ repo = make_repo( {".sampo/config.toml": "", "src/a.py": ""}, - deleted={".sampo/changesets/gallant-prince-ukko.md": ""}, documented_before={"README.md": "Changesets must live in `.sampo/changesets/`.\n"}, ) verdict, detail = run(repo)[".sampo/changesets/"] @@ -562,6 +567,26 @@ def test_the_posthog_case_an_empty_directory_is_not_a_deleted_one(make_repo: Cal assert "empty one is indistinguishable" in detail +def test_a_subsystem_removed_under_a_live_parent_is_broken(make_repo: Callable[..., Path]): + """The case the guard's own comment claimed to keep, and kept none of. + + `test_a_directory_whose_whole_tree_went_is_still_broken` below covers the parent going + too. `stacklok/toolhive` is the other shape and the commoner one: `pkg/container` is + alive and documented, and `pkg/container/verifier/` went in `7095e8e1` *"Remove + /verifier in favour of one coming from toolhive-core"*. Tracked parent, so the guard + fired, so the tool said nothing about a Sigstore verification subsystem its own + documentation still names two lines under a finding it DID report. + """ + repo = make_repo( + {"pkg/container/runtime.go": "", "src/a.py": ""}, + deleted={"pkg/container/verifier/sigstore.go": ""}, + documented_before={"README.md": "Verification lives in `pkg/container/verifier/`.\n"}, + ) + verdict, detail = run(repo)["pkg/container/verifier/"] + assert verdict is Verdict.BROKEN + assert "deleted in" in detail + + def test_a_directory_whose_whole_tree_went_is_still_broken(make_repo: Callable[..., Path]): """The recall this must not cost: requiring the PARENT to be tracked is what keeps a genuinely removed directory tree reportable.